Problem/Motivation

DuplicateContextualLinksTest::testSameContextualLinks fails randomly on CI with:

Error: Call to a member function isVisible() on null
DuplicateContextualLinksTest.php:58

The test places two identical views blocks (id => 'first' and id => 'second') and hardcodes #block-first as the container for the node contextual link. However, only one block actually renders the node teaser with its contextual link; the other gets an empty views-row. Which block gets the node depends on Block::sort(), which falls back to strcmp() on block labels when weights are equal. Since drupalPlaceBlock() generates random labels via randomMachineName(), the sort order varies between runs. When the node ends up in #block-second, the jQuery selector matches nothing, the click silently does nothing, and waitForElementVisible times out returning null.

Steps to reproduce

Run this test repeatedly.

It fails roughly 50% of the time depending on the random block labels generated.

Proposed resolution

Rewrite the test to not rely on rendering the same node, which has become problematic since #2940605: Can only intentionally re-render an entity with references 20 times.

Remaining tasks

Review and commit.

User interface changes

Nope.

Introduced terminology

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

N/A.

Issue fork drupal-3583187

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
amateescu’s picture

Title: Fix random test failure in DuplicateContextualLinksTest due to non-deterministic block sort order » [random test failure] DuplicateContextualLinksTest::testSameContextualLinks
smustgrave’s picture

But now do we need the first block?

amateescu’s picture

@smustgrave good question! We do need the first block because we're testing a scenario with duplicate contextual links using the same ID, and we should actually assert that the contextual links on both the first and the second block can be opened and closed individually.

However, while trying to do that, I discovered why this test suddenly started to fail so often, and it's because of #2940605: Can only intentionally re-render an entity with references 20 times. Since that issue, the node in our test is no longer being rendered in both views blocks, it now appears only the first block rendered on the page, which (funnily enough) is the _second_ views block, that's why my initial MR asserted that one :)

Investigating the problem caused by that issue should be done in a followup as it might take a very long time, but, until then, I've rewritten the test for the initial problem (#2891603: Contextual links can't handle multiple occurrences of the same contextual links (again)) to not rely on nodes rendered by views. To prove the new test actually catches the original bug, I've also opened a separate MR with the fix reverted.

amateescu’s picture

Issue summary: View changes

The test from the "prove new test without original fix" MR fails as expected: https://git.drupalcode.org/project/drupal/-/jobs/9257515

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

This lgtm.

Could we make sure to open the follow up per #8? It also occurred to me that it might be related to #3580855: Entity recursive rendering protection is not compatible with Fibers.

If someone else can +1 the RTBC, I can commit this.

smustgrave’s picture

+1 btw nice cleanup of the test to not need all those modules. Had to download locally to review gitlab diff had me crosseyed

  • godotislate committed cf83b856 on main
    test: #3583187 [random test failure] DuplicateContextualLinksTest::...

  • godotislate committed 46eed9dd on 11.x
    test: #3583187 [random test failure] DuplicateContextualLinksTest::...
godotislate’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Committed and pushed cf83b85 to main and 46eed9d to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

godotislate’s picture

I commented on #3580855-9: Entity recursive rendering protection is not compatible with Fibers to include investigation of the views blocks issue here in that issue, and to open a separate issue if they are not the same problem after all.

amateescu’s picture

Status: Fixed » Reviewed & tested by the community
Issue tags: -Needs followup

Commented on that issue as well. While that change fixes the original version of this test, I think we can write one that's more targeted without using views.

amateescu’s picture

Status: Reviewed & tested by the community » Fixed

Oops, didn't mean to change the status. But removing the tag was intentional since that issue is the followup :)

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.