Skip to content

Test link expansion inclusion rules with truth tables (direct links)#3886

Merged
nacnudus merged 1 commit intomainfrom
link-expansion-inclusion-tests
Mar 9, 2026
Merged

Test link expansion inclusion rules with truth tables (direct links)#3886
nacnudus merged 1 commit intomainfrom
link-expansion-inclusion-tests

Conversation

@nacnudus
Copy link
Contributor

Note: this only covers direct links. We'll cover reverse links separately.

This exhaustively tests the simplest case of link expansion, when there is a single candidate link to be included in a Content Store or GraphQL response. The simplicity makes it possible to test all possible combinations of properties of link sources and targets.

This differs from other tests because it specifically checks whether a link should be included, whereas other tests only check whether GraphQL does the same thing as classic link expanion, whether that's correct or not.

Drafts are currently only tested for classic link expansion. Once GraphQL supports drafts, a minor change to these tests will test GraphQL's link expansion too.

The more complex scenario, not addressed here, is when there are several candidate links. Precedence determines which of the candidates are expanded. But for a link to be considered for precedence, it must already pass the inclusion rules tested in this PR.

@nacnudus nacnudus force-pushed the link-expansion-inclusion-tests branch from c58106a to 12271ab Compare February 13, 2026 15:57
@nacnudus nacnudus force-pushed the link-expansion-inclusion-tests branch 2 times, most recently from 42a5437 to 5048790 Compare February 26, 2026 15:40
@nacnudus nacnudus changed the title Test link expansion inclusion rules exhaustively Test link expansion inclusion rules with truth tables (direct links) Feb 27, 2026
nacnudus added a commit that referenced this pull request Mar 2, 2026
Note: this only covers reverse links. Direct links will be done
separately (see #3886).

This exhaustively tests the simplest case of link expansion, when there
is a single candidate link to be included in a Content Store or GraphQL
response. The simplicity makes it possible to test all possible
combinations of properties of link sources and targets.

This differs from other tests because it specifically checks whether
a link should be included, whereas other tests only check whether
GraphQL does the same thing as classic link expanion, whether that's
correct or not.

Drafts are currently only tested for classic link expansion. Once
GraphQL supports drafts, a minor change to these tests will test
GraphQL's link expansion too.

The more complex scenario, not addressed here, is when there are several
candidate links. Precedence determines which of the candidates are
expanded. But for a link to be considered for precedence, it must
already pass the inclusion rules tested in this PR.
nacnudus added a commit that referenced this pull request Mar 2, 2026
Note: this only covers reverse links. Direct links will be done
separately (see #3886).

This exhaustively tests the simplest case of link expansion, when there
is a single candidate link to be included in a Content Store or GraphQL
response. The simplicity makes it possible to test all possible
combinations of properties of link sources and targets.

This differs from other tests because it specifically checks whether
a link should be included, whereas other tests only check whether
GraphQL does the same thing as classic link expanion, whether that's
correct or not.

Drafts are currently only tested for classic link expansion. Once
GraphQL supports drafts, a minor change to these tests will test
GraphQL's link expansion too.

The more complex scenario, not addressed here, is when there are several
candidate links. Precedence determines which of the candidates are
expanded. But for a link to be considered for precedence, it must
already pass the inclusion rules tested in this PR.
Copy link
Contributor

@mike3985 mike3985 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like it could've been simple to figure out 😓 Nice work.

I've left one comment, I think it'd be a worthwhile improvement.

Other than that, I do think there is room for improvement in readability / an opportunity for some of this code to be more self-documenting. But I'm happy to wait and see how it is to work with and to make improvements later, once more of the team have come into contact with it 👍

@permitted_unpublished_link_type = permitted_unpublished_link_type
end

attr_reader :with_drafts, :root_locale, :state, :renderable_document_type, :locale
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's considered idiomatic in Ruby to stick a trailing question mark on the end of boolean-returning predicate methods (I'd include with_drafts, included, etc in this group). That's going to be less convenient than declaring an attr_reader, but I think it's worth it.

def with_drafts?
  @with_drafts
end

There're probably one or two shorter-hand ways of achieving the same thing 🤔 I'd be equally happy with the approach above, though.

I'd go a step further than just the public predicate methods, too, and do the same for withdrawal and permitted_unpublished_link_type, because I think meeting our expectations for this particular naming convention will make this easier to read.

Note: this only covers direct links. We'll cover reverse links
separately.

This exhaustively tests the simplest case of link expansion, when there
is a single candidate link to be included in a Content Store or GraphQL
response. The simplicity makes it possible to test all possible
combinations of properties of link sources and targets.

This differs from other tests because it specifically checks whether
a link _should_ be included, whereas other tests only check whether
GraphQL does the same thing as classic link expanion, whether that's
correct or not.

Drafts are currently only tested for classic link expansion.  Once
GraphQL supports drafts, a minor change to these tests will test
GraphQL's link expansion too.

The more complex scenario, not addressed here, is when there are several
candidate links. Precedence determines which of the candidates are
expanded. But for a link to be considered for precedence, it must
already pass the inclusion rules tested in this PR.
@nacnudus nacnudus force-pushed the link-expansion-inclusion-tests branch from 5048790 to 91491b1 Compare March 8, 2026 19:35
nacnudus added a commit that referenced this pull request Mar 8, 2026
Note: this only covers reverse links. Direct links will be done
separately (see #3886).

This exhaustively tests the simplest case of link expansion, when there
is a single candidate link to be included in a Content Store or GraphQL
response. The simplicity makes it possible to test all possible
combinations of properties of link sources and targets.

This differs from other tests because it specifically checks whether
a link should be included, whereas other tests only check whether
GraphQL does the same thing as classic link expanion, whether that's
correct or not.

Drafts are currently only tested for classic link expansion. Once
GraphQL supports drafts, a minor change to these tests will test
GraphQL's link expansion too.

The more complex scenario, not addressed here, is when there are several
candidate links. Precedence determines which of the candidates are
expanded. But for a link to be considered for precedence, it must
already pass the inclusion rules tested in this PR.
@nacnudus nacnudus merged commit 68e3b57 into main Mar 9, 2026
17 checks passed
@nacnudus nacnudus deleted the link-expansion-inclusion-tests branch March 9, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants