Create a fault-tolerant method for interacting with links and fields in Javascript tests

I'm torn. Overriding the existing methods (option 1) is sort of a mess, since we'd have to intercept a lot of possible choices, and that probably gets ugly quickly. I'm also not exactly clear on the mechanism to try to intercept those methods, but I haven't looked that closely.

However, expecting test writers to know when to use clickLink() vs. safeClick() (option 2) seems somewhat unreasonable. Yes, it'd be great to have a generic solution, but it's not going to do much good if people don't know it exists and know when to use it. I'm barely getting started with FunctionalJS tests, and I sure as hell don't feel qualified to understand when a given command is likely to introduce a race condition or not. It's mostly all magic to me at this point. Maybe I (and others) would simply adopt the "always use the 'safe' option" approach, and maybe that's fine.

But yes, we clearly have a general race condition problem in our JS tests, and I'm glad we now have an issue to try to track it. Moving to the phpunit component (which seems more appropriate) and tagging for 'Random test failure' (since that's the class of issues this one is trying to solve.

A similar problem in the Media and Media Library.

https://www.drupal.org/pift-ci-job/1196544

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload
WebDriver\Exception\UnknownError: unknown error: Element ... is not clickable at point (628, 434). Other element would receive the click: ...

https://www.drupal.org/pift-ci-job/1190550

1) Drupal\Tests\media\FunctionalJavascript\MediaStandardProfileTest::testMediaSources
WebDriver\Exception\UnknownError: unknown error: Element  is not clickable at point (44, 588). Other element would receive the click: ...

Many random test failure.

Pretty cool that this works, but....

The issue I see here, is that this is just solving one very small fraction of possible causes of these random fails. There are an X number of reasons you need to wait during a javascript test (and even more for Nightwatch tests, which doesn't even pause on page loads). So are we going to try to catch ALL the reasons that can cause this, or are people going to need to learn that you need to waitForX a lot when writing these types of tests?
Annoying? Maybe, but it's the reality of writing javascript tests and catching one (or some) of the causes of this will not make them clearer to use or understand I think.

We don't want to get to a state where we are just debouncing EVERYTHING by 100ms like what was done in the MinkPhantomJSDriver (see https://github.com/jcalderonzumba/MinkPhantomJSDriver/pull/68). Taking away the 100ms led to a number of fails in our tests IIRC, so having that in was a solution to some of our random fails (just not a very good one I feel).

I just corrected some nitpicks:
- Fixed coding standards errors, except in the test. I think it doesn't need a doc comment (I couldn't come up with one).
- Minor modification to the JavascriptSession to be more similar to the parent Session class.

How do you test if it is working though?

Partially related but naming is often an issue too #2815297-14: Actions module should allow to edit and remove non-configurable actions

#12
Added the test.
I'm not sure if creating a new hidden testing module in the modules folder is okay?

Adding @group annotation to the test.

#14
Apparently it's not okay: ComposerIntegrationTest fails.

Thanks! Here's the fixed version.

+1 to the points in #24. When I was looking at #21 I thought the same thing.

Some additional nits and concerns:

1. +++ b/core/modules/system/tests/modules/js_click_test/js_click_test.info.yml
   @@ -0,0 +1,6 @@
   +description: 'Module for testing fault tolerant clicks.'
   

   fault-tolerant

   Also, maybe we should say "... in JavaScript tests" or something? we've got JS in the name, maybe worth being more explicit in this description, too?

2. +++ b/core/modules/system/tests/modules/js_click_test/src/Controller/JSClickTestController.php
   @@ -0,0 +1,69 @@
   +   * after a 2 second wait.
   

   Should this 2 second wait be a route parameter or something?

3. +++ b/core/modules/system/tests/modules/js_click_test/src/Controller/JSClickTestController.php
   @@ -0,0 +1,69 @@
   +            // Resize the box to cover the target link.
   +            'width: 100px;',
   +            'height: 40px;',
   +            // Position the box over the target link.
   +            'position: relative;',
   +            'top: -30px;',
   +            'left: -5px;',
   

   (In the new CSS file, not here, but...) don't we also want to set a z-index to force this to be in front of the target_link?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/FindJavascriptElementsTrait.php
   @@ -0,0 +1,29 @@
   + * Trait with finding extending NodeElements.
   

   Not sure what this means. Can we more clearly word what this trait is for? "finding extending" isn't proper, regardless.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/FindJavascriptElementsTrait.php
   @@ -0,0 +1,29 @@
   +  /**
   +   * The session.
   +   *
   +   * @var \Behat\Mink\Session
   +   */
   +  protected $elementSession = NULL;
   

   Unless I'm missing something, I don't see this actually being used anywhere, other than passing it around for the new constructors. Can't we remove this entirely, and majorly simplify lots of this?

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptNodeElement.php
   @@ -0,0 +1,49 @@
   + * Extends NodeElement to provide fault tolerant clicking.
   

   fault-tolerant.

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptSession.php
   @@ -0,0 +1,39 @@
   +  private $page = NULL;
   

   protected instead of private?

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/ExtendedSessionTest.php
   @@ -0,0 +1,43 @@
   + * Tests that session is extended and all find methods return extend elements.
   

   "return extend elements" doesn't make sense. Does this mean "return extended elements."? What does "extended" mean?

Okay, fixed everything, thanks!
The reason I added that inline code is because I thought keeping the test in one file would be simpler.

#25.5
I'm not sure how this could be simplified.

Thanks, that's much improved! Attaching the interdiff for other folks to review, too.

Re: #25.5: Indeed, I was missing something. ;) I didn't notice that we pass $session to the parent constructor (I was searching for "elementSession" not "session") and I now understand why that's needed. ;) Please ignore.

Two tiny remaining nits:

1. You added 2 new libraries for this, one for JS the other for CSS. Seems like a bit of overkill, and they could be combined into a single library. Not going to NW for this, but if you want to re-roll (or if anything else of substance turns up), maybe this could be simplified.
2. Now that it's a URL param (thanks!) I wonder if we really need these tests to wait 2 seconds each. ;) Maybe 1000 is a better value? Just trying to look out for the throughput of the test bots, since every delay is compounded a lot by how often these tests are going to be invoked.

Thanks!
-Derek

Adding the fix

Did you mean 100 vs 1000? If 100 is still enough to "see" the bug, that's great. Just double checking.

Otherwise, I have no more concerns. RTBC to my eyes.

Thanks!
-Derek

I think it's enough.
Uploading the testonly to prove it :)
(If it fails it can go back to RTBC)

Hrm, bummer, that passed. ;)

Can we see maybe 3 test-only versions @ 500, 1000 and 2000 and then decide?

Sorry for the hassles/churn!

Thanks,
-Derek

Let's see...

Whoops, didn't actually mean to assign to myself.

p.s. Now that I applied locally, I saw that the test is at:
core/tests/Drupal/FunctionalJavascriptTests/Tests/JSClickTest.php
while the module it depends on is in:
core/modules/system/tests/modules/js_click_test
I definitely don't grok how we decide what tests live in system vs the core/tests/Drupal world, but I wonder if the test class should move to:
core/modules/system/tests/modules/js_click_test
core/modules/system/tests/src/FunctionalJavascript/JSClickTest.php
?

I guess this is a test for the testing framework itself, not a test for something in Drupal, so the current location makes sense. But I wanted to ask.

Thanks,
-Derek

p.p.s. I regret rushing to upload the patches in #33 without also including a change to drupalci.yml to only run this test. :/ Whoops!!

Cool, that's more like it. ;) Let's see about a sweet spot between 100 and 500, then.
This time, with a change to drupalci.yml to only run this particular test, but to repeat it 20 times.

Hrm, what about 100ms x 20 repeats? Also 50 and 10 for good measure. ;)

Meanwhile, I noticed the bot complaining about this:

var/lib/drupalci/workspace/jenkins-drupal_patches-86345/source/core/tests/Drupal/FunctionalJavascriptTests/Tests/JSClickTest.php ✗ 1 more
24	@expectedException tags should not be used, use $this->setExpectedException() or $this->expectException() instead

So I fixed that (I think). Let's see what the bot says now.

(sorry for the noise, but I think this is important to find a reasonable value for this).

Hah! ;) So I guess 100 is about right, after all. LOL ;) Maybe we should bump to 150 or so. Committers could easily change the value on commit based on all these test results. But here's a full patch with the fix to using setExpectedException(), back at 100ms. Probably RTBC but I'll let @tedbow (or someone) do that.

Thanks,
-Derek

Nice! Those are the test results we were hoping for. ;)

I obviously can no longer RTBC this, but I believe it's ready for that.

Note to committers: please list @tedbow and @bendeguz.csirmaz ahead of me in the commit message. I'm only at the front of the default list as a result of uploading all these test-only patches to find/confirm the right value. Those two actually wrote all the code that matters in here.

Thanks,
-Derek

Thanks for doing this!
Since 100 randomly failed once I think it should be a greater value to be safe.

Well, 100 randomly *passed* for the test-only. ;) To consistently see the test-only fail, we need more than 100, but that's not really the point. We just wanted to make sure this fix is working, and it is.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptNodeElement.php
@@ -0,0 +1,49 @@
+      catch (UnknownError $exception) {

We've discovered that this Exception class is not reliable in #3056536: LayoutBuilderDisableInteractionsTest randomly fails. It needs to be \Exception because on MacOS connecting directly to chromedriver that's the exception that's thrown. On Linux and via selenium-server it's a UnknownError but this will fail on many people's local testing environments. As you're checking the error message anyway I think you'll get the behaviour you want with catch (\Exception $e)

Making this critical because atm you cannot run layout builder tests locally without it failing if you have a fast machine. Plus DrupalCI can't run JS tests with a concurrency of 1 without failing. Going to mark #3246891: Layout Builder JS tests are failing locally and on DrupalCI with a concurrency of 1 as a duplicate.

The patch in #53 also adds protection for NodeElement::setValue() as that was also causing not interactable errors for Layout Builder tests.

Fixing the tests and removing usage of the deprecated jQuery.once library.

Let's handle the visible issue...

Looking at this change we can move all the hackery of the wait into something we're already overriding - the DrupalSelenium2Driver.

Hopefully this will fix the remaining errors.

Updated issue summary with new information about how to cause the problem and the nature of the fix.

Locally I can get the AjaxBlockTest is I use sqlite. The attached patch fixes this.

Okay I'm going to stop trying to fix:

• Drupal\Tests\layout_builder\FunctionalJavascript\ContentPreviewToggleTest
• Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderTest
• Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlockTest

We should file follow-ups to explore fixing those. The current approach here of making link clicking and field value setting more resilient reduces the number of random JS errors considering as we can see from the attached patches.

I've also added tests for the form value setting and renamed things in the tests so they are not so click focussed and I removed the dependency on jQuery because why not.

Here's what the different patches attached show:

• 3246891-4.test-only.patch: this is how bad things are on HEAD when JS tests are run with a concurrency of one - should have the most fails.
• 3032275-69-concurrency-one.patch: this shows how this change improves under the concurrency of one condition

Yay another difference between a modern chromedriver and the one we currently test on using DrupalCI - we need to update it sometime.

@alexpott: Thanks for resurrecting this issue and moving it so far forward!

A few trivial docs nits fixed with attached patch. Then a few other comments / concerns. Only #3 prevents me from recommending RTBC, but I'd love feedback on the others.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
   @@ -134,4 +137,96 @@ public function uploadFileAndGetRemoteFilePath($path) {
   +  public function click($xpath) {
   

   #2946294: Fix race conditions in OffCanvasTestBase makes me wonder if we also want to override Behat\Mink\Element\TraversableElement::clickLink(), too. But that'd require undoing much of the goodness / simplification from #62...

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
   @@ -134,4 +137,96 @@ public function uploadFileAndGetRemoteFilePath($path) {
   +   * Waits for an element(-s) to appear and returns it.
   +   *
   +   * @param int|float $timeout
   +   *   Maximal allowed waiting time in seconds.
   +   * @param callable $callback
   +   *   Callback, which result is both used as waiting condition and returned.
   +   *   Will receive reference to `this driver` as first argument.
   

   These docs are a bit clumsy and hard to follow.

   A) "element(-s)" vs. "it" - if we're going to handle single or plural, "it" should probably be "it/them".

   B) "which result is both used as a waiting condition and returned" is confusing. And maybe now wrong? Looks like the callback is expected to return TRUE if it found the thing, but we don't actually care about or return the underlying result beyond that.

   That said, this is a private helper in a dark corner of the test plumbing, so maybe it doesn't matter. ;)

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptSession.php
   @@ -0,0 +1,39 @@
   +class JavascriptSession extends Session {
   

   It appears that we removed the code that was registering this in WebDriverTestBase. It seems this is no longer needed. If so, we should remove it.

   Looks like how it was registered was changed in #53, but then completely removed in #62.

Thanks again!
-Derek

Thanks for the review @dww

Re #73.1 All clicks will be more fault tolerant now. If they are being added by ajax you're going to need wait otherwise the find will fail but I think that that is okay.

#73.2 and they are copied from upstream docs. I've improved them a bit. Re part B the "which result is both used as a waiting condition and returned" is correct. The callback is expected to return something truthy to end the waiting and something falsey to continue to wait.

#73.3 yep this is dead code now.

Thanks, @alexpott!

1. Great! After I posted 73, I was wondering if that’d be the case, but I didn’t have time to dig in and investigate before bed. thanks for confirmation.

2. New docs look good. Truthy or not is the key point, which it now says. Thanks.

3. Yay, glad to see it go.

I have nothing other than too many patches uploaded here stopping me from RTBC. 😉 hopefully @tedbow will RTBC soon (he mentioned in Slack he was planning to review this today).

Thanks again,
-Derek

Thanks for the review, @tedbow! All good points.

But first, I actually tried running the test locally while I was working on the proposed changes. Discovered we never fully addressed #50. The test was dying for me (MacOS chrome + chromedriver 94.0.4606.81) like so:

There were 2 failures:

1) Drupal\FunctionalJavascriptTests\Tests\JSInteractionTest::testNotClickable
Failed asserting that exception of type "Exception" matches expected exception "WebDriver\Exception". Message was: "element click intercepted: Element <a href="/drupal-9_1/js_interaction_test/100" data-drupal-selector="edit-target-link" id="edit-target-link">...</a> is not clickable at point (42, 17). Other element would receive the click: <div class="blocker-element" data-drupal-selector="edit-blocker-element"></div>
  (Session info: chrome=95.0.4638.69)
  (Driver info: chromedriver=94.0.4606.61 (418b78f5838ed0b1c69bb4e51ea0252171854915-refs/branch-heads/4606@{#1204}),platform=Mac OS X 10.15.7 x86_64)" at

I had to change both core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php and core/tests/Drupal/FunctionalJavascriptTests/Tests/JSInteractionTest.php to use \Exception not WebDriver\Exception. Hope that's cool.

Uploading that here separately, before I work on #76 so there's a separate patch / interdiff in case we want to change course on this.

This fixes most of #76. I'm leaving point 3 for @alexpott to decide. Would like confirmation on 6, too.

1. 👍😉
2. Simplified to "Size".
3. Leaving for Alex. @todo (maybe)
4. I'm pretty used to core JS tests with "target" and "trigger" terminology. But I see your point. Per Slack, I think verb-first is more clear, so I went with "Remove Blocker Trigger"). Updated everything to match, test is still passing locally. Hopefully easier to read / understand.
5. Went with "Enable Field Trigger".
6. Seems like an excellent reason to be passing $this into that callback (which we're already doing). We don't need this to be static (and therefore public). Agreed that less API is better here. If we have some public need for this (hard to imagine), we can change it then.
7. Changed to "Assert an exception is thrown when the field is never enabled."

@dww re #77 I would love to understand why you're not getting a typed exception. Imo the change to \Exception is not correct and we need to work out what it is about the way you have things set up that results in an un-typed exception. This is because there are other tests that capture even more specific exceptions that would be broken.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
@@ -225,7 +225,7 @@
-  public static function isExceptionNotClickable(\Exception $exception): bool {
+  private function isExceptionNotClickable(\Exception $exception): bool {

We should move this elsewhere. And leave it as a public method. There are other places in the codebase that need this. Plus this should stay as static wherever it goes because it has no need for $this.

@dww I use OSX and chromedriver and \WebDriver\Exception is what I'm getting. Can you provide a full backtrace for your exception so we can see what code is converting it to \Exception - I can not see where that is coming from and rather than converting this to the catch all I'd like to understand it. (Therefore the patch here reverts this change).

Fixed up the wait to be hard-coded - I agree that having this as a setting is OTT.

Also move the exception message tester to JsWebAssert - couldn't think of a better place and used it in the other place where it needs to be.

Yeah, I don't think #77 is the right approach. Happy to see it reverted. But for reference, attaching the full phpunit output. Here's what's going on for me locally that's throwing \Exception in my case:

vendor/instaclick/php-webdriver/lib/WebDriver/Exception.php

    public static function factory($code, $message = null, $previousException = null)
    {
        // unknown error
        if (!isset(self::$errs[$code])) {
            if (trim($message) === '') {
                $message = 'Unknown Error';
            }

            return new \Exception($message, $code, $previousException);
        }
        ....
    }

I'm getting "element not interactable" but that's not in the list of exceptions that class understands. My (unmodified) composer.lock says:

            "name": "instaclick/php-webdriver",
            "version": "1.4.10",

Maybe it's a mismatch between chrome (95.0.4638.69) and chromedriver (94.0.4606.61)? Sorry, don't have time to dig deeper right now.

@dww I have no idea how but your vendor directory is very very out-of-date. The current code for 1.4.10 is

    /**
     * Factory method to create WebDriver\Exception objects
     *
     * @param integer    $code              Code
     * @param string     $message           Message
     * @param \Exception $previousException Previous exception
     *
     * @return \Exception
     */
    public static function factory($code, $message = null, $previousException = null)
    {
        // unknown error
        if (! isset(self::$errs[$code])) {
            $code = self::UNKNOWN_ERROR;
        }

        $errorDefinition = self::$errs[$code];

        if (trim($message) === '') {
            $message = $errorDefinition[1];
        }

        if (! is_numeric($code)) {
            $code = self::W3C_WEBDRIVER_ERROR;
        }

        $className = __CLASS__ . '\\' . $errorDefinition[0];

        return new $className($message, $code, $previousException);
    }

and https://github.com/instaclick/php-webdriver/blob/1.4.10/lib/WebDriver/Ex...

Yeah, my local vendor was all messed up from switching branches and not re-running composer install. Sorry about that! Confirmed that #81 is now passing locally:

Testing Drupal\FunctionalJavascriptTests\Tests\JSInteractionTest
...                                                                 3 / 3 (100%)

Time: 1.27 minutes, Memory: 4.00 MB

OK (3 tests, 11 assertions)

Although I've uploaded a lot of patches here, many of them were just trying to get the testbot to give us results. The latest code here is mostly @alexpott's additions to and rewrites of the original work by @tedbow and @bendeguz.csirmaz. So I'm gonna be bold and RTBC this. Fixing this would be a huge win for core development, so no reason to further delay.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -506,4 +508,18 @@ public function assertNoElementAfterWait($selector_type, $selector, $timeout = 1
+  public static function isExceptionNotClickable(\Exception $exception): bool {

Was this left as the untyped exception on purpose?

That's a great catch @lauriii. Fixing that up. It's a minor change so leaving at rtbc.

Thank you for the new patch @alexpott! Committed 7ff9cbf and pushed to 9.4.x. Thanks!

Leaving open for 9.3.x backport.

Backporting to 9.3.x since this only impacts tests. Also got confirmation from @catch on Slack that he didn't have any concerns on backporting this.