Problem/Motivation
ContextualLinkClickTrait::clickContextualLink() was introduced to ease the interaction with contextual links in tests.
However, in some cases when using it multiple times within a single page load it can produce unexpected results.
Proposed resolution
Remove the ambiguity around toggling the .visually-hidden
class, and with pressing the .contextual button
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#27 | 2918718-27.patch | 3.11 KB | smustgrave |
| |||
#27 | interdiff-25-27.txt | 1.33 KB | smustgrave |
#15 | 2918718-15.patch | 1.99 KB | karishmaamin |
#6 | 2918718-6.patch | 1.99 KB | Anonymous (not verified) |
Comments
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedIn #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD we found an unstable works of the Mink press(). So we had to replace it like:
Also sometimes a bug with a non-working button on the first click can be reproduced manually on real sites. A few clicks help.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commented#2:
Separate issue for this #2926207: Contextual button sometimes not works. Bug in Chrome?.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedAfter #2870452-39: Convert web tests to browser tests for menu_ui module (yes, I was not mistaken in the link) the
clickContextualLink()
will have to work much more stable. Because it will be independent of the initialization of contextual links.Another source of instability for PhantomJS tests is
But this should not be a problem for WebDriver (see #2936122-6: Find out why JavscriptTestBase occasionally needs a waitForElement on a normal page load).
Therefore, I hope soon this issue will be no longer relevant.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commented#2924201-56: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD showed that the reliability promised by me in #5 did not come 😓
Why? Having cried well, I found few points:
Point 1.
It switches the class regardless of its current state. If the button is visible, it will hide it. This is what can happens in 2924201-56.
Point 2.
Remove a
".visually-hidden"
class does not guarantee that the button will immediately become visible. Because css transition (delay, duration) and other effects. See #2901792: Disable all animations in Javascript testing. Therefore, we should wait for this moment. This is what can happens in 2924201-56 too.Point 3.
Point 2 refers to the contextual links that appear after clicking on the button.
Point 4.
If a general selector is used, resulting in several suitable context regions, a conflict occurs.
The attached patch struggles with all these points.
PS.
In general, this is still not an absolutely reliable method, because
We check the existence of a
".contextual-links"
class that appears even beforedrupalContextualLinkAdded
event. So, it is possible that a full-fledged readiness of contextual links has not yet come. It would be great if the contextual, quikedit, settings tray modules added some classes, meaning that they have already set up for this contextual link. But this is already beyond the scope of this issue.Comment #14
larowlanComment #15
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 9.4.x. Please review
Comment #17
yogeshmpawarComment #18
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRe-rolled patch against #15 with interdiff for 9.4.x. Please review
Comment #20
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #21
larowlanComment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedPatch or MR doesn't apply anymore
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.
Comment #25
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs commentedRerolled for 10.1.x.
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #28
larowlanThis looks like a nice step forward
Comment #29
xjmBumping up from "minor"; any bugfix that means less random race condition stuff in our JS tests is probably at least major if it works as advertised.
This contains a grammatical error. I'd suggest:
Could we put some debug information in this exception message, like
$selector
for example?Also, there's a grammatical error. I'd suggest:
Don't we usually do
waitForElementVisible()
rather than waiting a magical 10 seconds? Tagging for FEFM review on this point.This comment could be expanded a bit as I'd have no idea what it meant if I hadn't read this issue.
This might also help with some of the issues in #3267247: [meta] Fix and re-enable tests skipped for random failures. We should test those tests with this change.