Intermittent failure in MediaUiJavascriptTest

Nice work @benjifisher -- looks like #2 caught one! https://www.drupal.org/pift-ci-job/853413

This means that the random fail is already in HEAD, and not introduced by #2934424: Media has no collection route. So we can go ahead with that issue, and instead need to sort out which previous commit introduced this random fail.

Setting back to NW now that we've confirmed the random fail exists in HEAD.

It looks like this might be straightforward to fix. From the test:

    // Test the creation of a media type using the UI.                          
    $name = $this->randomMachineName();
    $description = $this->randomMachineName();
    $this->drupalGet('admin/structure/media/add');
    $page->fillField('label', $name);
    $machine_name = strtolower($name);
    $this->assertJsCondition("jQuery('.machine-name-value').html() == '$machine_name'");
    $page->selectFieldOption('source', 'test');
    $this->assertJsCondition("jQuery('.form-item-source-configuration-test-config-value').length > 0;");
    $page->fillField('description', $description);
    $page->pressButton('Save');
    $assert_session->pageTextContains('The media type ' . $name . ' has been added.');

That's totally missing any wait condition above the last line, so we need to add that to ensure that the request completes before the assertion runs.

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php
@@ -63,6 +63,8 @@ public function testMediaTypes() {
     $page->pressButton('Save');
+    $assert_session->waitForLink('Add media type');
+    $assert_session->addressMatches('/^\/admin\/structure\/media$/');

@@ -134,6 +136,8 @@ public function testMediaTypes() {
     $page->pressButton('Save');
+    $assert_session->waitForLink('Add media type');
+    $assert_session->addressMatches('/^\/admin\/structure\/media$/');

Exactly for the reason you pointed out in #10, another developer is likely to come here in the future and just remove them, without even noticing a failure :) How about a comment before these lines, explaining the need and referring to this issue URL?

Note: it surprises me too that we need to wait for the element, on a non-AJAX call, but I have no other suggestion to make, unfortunately. :)

Also, in other tests, after waiting for an AJAX interaction, we assert the new element is really present, with something like:

    ...
    $page->attachFileToField('files[field_media_image_0]', \Drupal::root() . '/core/modules/media/tests/fixtures/example_1.jpeg');
    $result = $assert_session->waitForButton('Remove');
    $this->assertNotEmpty($result);

I'm no test expert, so may have nothing to do, just wanted to mention it for consistency with other tests.

Note: it surprises me too that we need to wait for the element, on a non-AJAX call, but I have no other suggestion to make, unfortunately. :)

It surprises me too, but it's not the first time this is needed, see #2864177: Random failure in FormErrorHandlerCKEditorTest
But it would be nice to find out why and when this is needed.....

Also, in other tests, after waiting for an AJAX interaction, we assert the new element is really present

This is correct. You need to assert that the element was actually found, otherwise you might just be waiting for 10 seconds, which might help the test pass, but is fairly inefficient. We want to be sure we wait for as short a time as possible.

I feel we do need the assertion that $assert_session->waitForLink('Add media type'); found something, because if we change the link text to something else later, we have no indication that this test is running 10 seconds longer then necessary.

For reference if you change the waitFor to
$assert_session->waitForLink('Dingoes ate my baby');
the test passes just fine now, it just runs a little longer.

So I think we need to future proof this and assert that we wait for the correct link.

@benjifisher thanks for adding it :)

We have proof of failure in #2, we have proof of passing in #9, nice work!

I queued several more runs of #9; the fail in #2 only showed up 1/6 times, which means we need several times that to be reasonably confident it is indeed fixed. :)

Another good way to test these issues is to come up with a patch that fails 100% of the time (e.g. with a very high sleep at exactly the step where we're waiting for the page load). Not a requirement, but helpful. We can also just wait to see what happens in like 20 runs. (I'm only queuing a few at a time to avoid overloading CI; will revisit later.)

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php
@@ -63,6 +63,11 @@ public function testMediaTypes() {
+    // https://www.drupal.org/project/drupal/issues/2934997.

@@ -134,6 +139,11 @@ public function testMediaTypes() {
+    // The wait prevents intermittent test failures. See
+    // https://www.drupal.org/project/drupal/issues/2934997.

Oh, we should never put "See" to the issue in a patch. If people want to know what issue added the line, they can use git blame.

I tried to make this test fail (without the patch ) by just adding sleep(1) to index.php.

It didn't fail so I went to 5 and then 10. It then failed but in a different spot:

Javascript condition met:
jQuery('.form-item-source-configuration-test-config-value').length > 0;
Failed asserting that false is true.
/var/www/d8/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/var/www/d8/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php:121
/var/www/d8/core/modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php:63

$page->selectFieldOption('source', 'test');
    $this->assertJsCondition("jQuery('.form-item-source-configuration-test-config-value').length > 0;");

Assuming this is some kind of ajax for reload where option show up after you select a value for field. Does this have to be done with assertJsCondition?

Could it be done with \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElement()? Of course checking for result.

but this is what is making this test fail. but the fact that it only fails after waiting 10 seconds and not on the same line makes me wonder if there is not something else going on besides just slow tests.

Having a 100% failing test might be nice to have but sounds unlikely to happen. Since it's still unclear why this test fails in the first place. Having to wait for a non-dynamic element on page load is not something that is usually needed. That might be something we want to look into, since it is now needed twice in core (so I assume we will run into this again), but seems out of scope for fixing this critical. I opened up a follow up for this, #2936122: Find out why JavscriptTestBase occasionally needs a waitForElement on a normal page load

Given the 1/600 failure rate in #2, it means the 2200 runs in #9 mean we now only have 0.0455% chance of not having caught the cause but still get 2200 passes. Sounds pretty solid to me.
Bumping back to RTBC to see if people agree :)

Committed 14c499f and pushed to 8.6.x. Thanks!

Will cherry-pick to 8.5.x after the alpha freeze.

Done.