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
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
Comment #3
amateescu commentedComment #4
amateescu commentedComment #5
amateescu commentedComment #6
smustgrave commentedBut now do we need the first block?
Comment #8
amateescu commented@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.
Comment #9
amateescu commentedThe test from the "prove new test without original fix" MR fails as expected: https://git.drupalcode.org/project/drupal/-/jobs/9257515
Comment #10
godotislateThis 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.
Comment #11
smustgrave commented+1 btw nice cleanup of the test to not need all those modules. Had to download locally to review gitlab diff had me crosseyed
Comment #16
godotislateCommitted and pushed cf83b85 to main and 46eed9d to 11.x. Thanks!
Comment #18
godotislateI 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.
Comment #19
amateescu commentedCommented 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.
Comment #20
amateescu commentedOops, didn't mean to change the status. But removing the tag was intentional since that issue is the followup :)