Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

I think there might be another issue that addresses this by disable css transitions in tests

A few of today:

https://www.drupal.org/pift-ci-job/1295045
https://www.drupal.org/pift-ci-job/1295055
https://www.drupal.org/pift-ci-job/1295053
https://www.drupal.org/pift-ci-job/1295722

Testing Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
.....E.                                                             7 / 7 (100%)

Time: 2.69 minutes, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload
WebDriver\Exception\UnknownError: unknown error: Element is not clickable at point (188, 631)

One more here https://www.drupal.org/pift-ci-job/1295943 . Is #2829040: [meta] Known intermittent, random, and environment-specific test failures still a thing? Shall this issue be added to that list?

Thanks, I added this issue to the list.

@alexpott put a screenshot of the failing spot in slack and it is inside a dialog, so might also be worth taking another look at #2856047: Avoid random failures in JavascriptTestBase when testing functionality in a dialog

This is now a follow-up of #2901792: Disable all animations in Javascript testing as that issue solves the majority of random fails that occur in MediaLibrary test. There might be others but they become way way rarer once that patch is applied. Postponing this issue on that one.

#2901792: Disable all animations in Javascript testing has been committed.

The problem was also found a few months earlier (headless chrome=62.0.3202.94, chromedriver=2.33.506092):
https://www.drupal.org/pift-ci-job/1196544

Automatic re-testing. Updated 10 Feb 2019 at 14:53 CET.

 Testing Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
...E                                                                4 / 4 (100%)

Time: 3.11 minutes, Memory: 4.00MB

There was 1 error:

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: ...
  (Session info: headless chrome=62.0.3202.94)
  (Driver info: chromedriver=2.33.506092 (733a02544d189eeb751fe0d7ddca79a0ee28cce4),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)

/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/Exception.php:155
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:157
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:218
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/Container.php:224
/var/www/html/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:775
/var/www/html/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:763
/var/www/html/vendor/behat/mink/src/Element/NodeElement.php:153
/var/www/html/vendor/behat/mink/src/Element/NodeElement.php:161
/var/www/html/vendor/behat/mink/src/Element/TraversableElement.php:115
/var/www/html/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php:395

Maybe related to #3032275: Create a fault-tolerant method for interacting with links and fields in Javascript tests.

I updated issue summary. It is possible that it will not occur anymore. #2901792: Disable all animations in Javascript testing could solve the problem but we should track this issue.

This is not happening anymore.

Correct status...

I ran into this twice today, once locally, once on a patch

Testing Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
.....E.                                                             7 / 7 (100%)

Time: 2.63 minutes, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload
Behat\Mink\Exception\ResponseTextException: The text "image-1.png" appears in the text of this page, but it should not.

/var/www/html/vendor/behat/mink/src/WebAssert.php:787
/var/www/html/vendor/behat/mink/src/WebAssert.php:279
/var/www/html/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php:969

I've run into this with Layout Builder, where occasionally $assert_session->assertWaitOnAjaxRequest(); is not sufficient before calls to pageTextContains()/pageTextNotContains. Any uses of those functions directly following $assert_session->assertWaitOnAjaxRequest() have been changed to equivalents that make multiple attempts before failing.

Test is further refactored to remove assertWaitOnAjaxRequest() wherever possible in favor of what changes are actually being looked for.

There were some instances where assertWaitOnAjaxRequest() was still required, but it is now supplemented with additional assertions that wait for changes that may occur after assertWaitOnAjaxRequest() is complete.

Lets give it another try...

Looks like the patch times out in the early 200s so reducing the repeats to 200 and cleaned up a few redundant calls and moved a few assertions that were not in the same order as the original test.

🤷‍♂️

I still think we need to go through this test and remove all assertWaitOnAjaxRequest().

testWidgetUpload doesn't pass locally for me without the attached fix. My local setup is quite fast because I don't use any virtualisation.

A version with every assertWaitOnAjaxRequest() removed is on the way

@bnjmnm oh sorry - I've been working on that too :( so we can compare patches. When we both post.

I've tested this patch with and without a 2 second wait in index.php - doing that is a good way to prove that a slow AJAX request won't break a test. One of the thing I discovered was that the added \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::waitForNoText() was not working as expected.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -556,7 +556,6 @@
-    $this->assertSame('Dog', $page->findField('Name')->getValue());

This assert turned out to the just wrong. When you filter and then change view the Name field is reset.

@alexpott I'm all for working on this simultaneously and comparing patches. Based on your interdiffs it looks like we're targeting different things at the moment anyway. Thanks for jumping in on this huge patch.

Regarding the fail in #27 -- in earlier iterations of this patch I found that waiting for text to appear/not appear is not reliable after file uploads or "Save and Select/Insert", which is why those particular assertWaitOnAjaxRequest() calls hadn't been removed.

In the patch I'm working on, I think I've addressed the uncertainly when calling attachFileToField(), using $this->addMediaFileToField() instead. This ensures the file is actually uploaded before proceeding, something that the presence of text doesn't guarantee.

These are the new methods

/**
   * Attach file to file field with specified locator.
   *
   * @param string $locator
   *   Input id, name or label.
   * @param string $path
   *   Path to file.
   * @param string $completed_locator
   *   Locator to look for once upload is complete.
   * @param bool $expect_completion
   *   If the uploaded is expected to be successful.
   *
   * @todo replace with whatever gets added in
   *   https://www.drupal.org/node/3061852
   */
  public function addMediaFileToField($locator, $path, $completed_locator = '.media-library-add-form__added-media', $expect_completion = TRUE) {
    $page = $this->getSession()->getPage();

    $this->waitForFieldExists($locator);
    $file_storage = $this->container->get('entity_type.manager')->getStorage('file');
    $current_number_of_files = count($file_storage->loadMultiple());
    $page->attachFileToField($locator, $path);
    if ($expect_completion) {
      $this->waitForFileUploaded($current_number_of_files);
    }
    $this->assertElementExistsAfterWait('css', $completed_locator);
  }

  /**
   * Waits to see if a new file has been uploaded.
   *
   * @param int $number_of_files
   *   The number of files prior to the upload.
   * @param int $timeout
   *   (Optional) Timeout in milliseconds, defaults to 10000.
   *
   * @throws FileNotExistsException
   *   If no new file uploaded.
   *
   * @todo replace with whatever gets added in
   *   https://www.drupal.org/node/3061852
   */
  protected function waitForFileUploaded($number_of_files, $timeout = 10000) {
    $start = microtime(TRUE);
    $end = $start + ($timeout / 1000);

    $file_storage = $this->container->get('entity_type.manager')->getStorage('file');

    do {
      $new_number_of_files = count($file_storage->loadMultiple());
      if ($new_number_of_files > $number_of_files) {
        return;
      }
      usleep(100000);
    } while (microtime(TRUE) < $end);

    throw new FileNotExistsException("The file was not uploaded.");
  }

I'm now working on something similar for "Save and Select/Insert"

@bnjmnm for me this is just another instance of not waiting for something new!

     // Ensure the media item was saved to the library and automatically
     // selected. The added media items should be in the first position of the
     // add form.
     $this->waitForText('Add or select media');
     $this->waitForText($png_image->filename);

Both bits of text are also on the previous form that asks for the alternate text.

Let's assert for somethings that's actually a result of pressing save and select.

Fixing waitForText to not emit 20000 as the message. And to wait for the 20secs. It's seems a very long time to wait but it is also very surprising that the text '1 of 2 items selected' has not appeared after 10 seconds.

Yet another set of asserts for things that are there before and after pressing on a button.

#34 is finally green on sqlite with 50x - lets try with 200x.

Adding some instrumentation so we can see what's happening.

From slack:

quietone 0 h 45
Getting an unrelated to my patch test failure with postgresql from MediaLibraryTest.php. I've run it twice, same result. Just thought I'd let you now. https://www.drupal.org/pift-ci-job/1403588

And my reply:

$assert_session->elementExists(‘css’, ‘.media-library-item__remove’)->click();
$assert_session->assertWaitOnAjaxRequest();
$assert_session->pageTextNotContains($png_image->filename);

the code probably needs to be updated to not use $assert_session->assertWaitOnAjaxRequest();
but rather wait until $assert_session->pageTextNotContains($png_image->filename); is TRUE
not 100% sure, but $assert_session->assertWaitOnAjaxRequest(); can sometimes return early, or have unintended interactions.

It looks like a lot of work has already been done here.

Most of the random fails can be addressed by replacing $assert_session->assertWaitOnAjaxRequest() with assertions that check for the presence/absence of elements. There is one aspect of this that can't be addressed in this manner, which has spun off into its own issue #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest) -- there appear to be random failures when uploading files to the media widget multiple times within the same test. That underlying issue would need to be addressed before MediaLibraryTest can work consistently.

Probably should have done this when #40 was added, but it's more accurate to set this to postponed and waiting on #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest). There are issues specific to making multiple file uploads in the same FunctionalJavascript test that cause intermittent failures. If the MediaLibararyTest is refactored to address all other intermittent failures, there will still be intermittent failures due to this upload issue, something that (as proven in the issue) is not due to the way MediaLibraryTest is written.

If issue #3066447 is not fixable, it may be possible to address this by splitting the tests in MediaLibararyTest into many smaller ones so there are fewer file uploads inside a single test method,

Discussed this with @xjm and it was decided to not block this on #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest) as the changes here will reduce the number of random failures, even though that issue will need to be addressed for them to be removed entirely.

I have a version of this patch with significantly fewer errors that was being tested in a burner issue to avoid clogging this one with dozens of trial/error patches. Once I get that rerolled I'll set this back to Needs Review.

Most of the random fails can be addressed by replacing $assert_session->assertWaitOnAjaxRequest() with assertions that check for the presence/absence of elements.

There are other buttons too that don't have $assert_session->assertWaitOnAjaxRequest() that need the next interaction item to wait to appear.

This addresses random errors in MySQL and Postgres. Unfortunately, there are still intermittent errors in SQLite that probably can't be addressed until #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest) is completed. I added the WITH-DATABASE-CHECKS tests to help demonstrate where it is failing. After a certain amount of uploads (varies on each instance), the media item never gets added to the database. Once this happens, the test errors may report different points of failure (explained further in #3066447) but the root cause appears to be the media item never getting added to the database.

Still, this significantly reduces intermittent errors, and never relies solely on $assert_session->assertWaitOnAjaxRequest() before proceeding to a next step. Note that $assert_session->assertWaitOnAjaxRequest() is still in use in a few places as it appears to be necessary for form ids and label 's for attributes to match (this information is also in the comments). There may be a solution I overlooked, but I'm also in support of having that be a followup issue.

This patch is a magnificent piece of work. It decreases the verbosity of MediaLibraryTest while increasing its reliability, and lays the groundwork for a trait that will enable other tests to meaningfully interact with the media library. I'm sure that a lot of my feedback is not valid, and a lot of it is certainly nitpicky -- give me a break, though: it was hard to find things to dislike here!

1. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -51,6 +51,10 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +    if (isset($this->view->args[0])) {
   +      $form['#attributes']['data-drupal-media-type'] = $this->view->args[0];
   +    }
   

   This could use a comment.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -432,23 +427,19 @@ public function testWidget() {
   +    $this->mediaLibraryOpenButton('field_unlimited_media');
   

   Oh my God...this is so much better and more readable. Thank you. 🙏

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -507,18 +492,15 @@ public function testWidget() {
   -    $assert_session->assertWaitOnAjaxRequest();
   -    $assert_session->buttonExists('Hide row weights')->press();
   

   👍 I see why this was removed; our very next action is to visit a different URL, so there's no point in hiding the row weights.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -556,30 +532,26 @@ public function testWidget() {
   +    // Additional timeout to address random fails on SQLite tests.
   +    $this->waitForNoText('Bear');
   

   We probably need a follow-up @todo for this...?

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -556,30 +532,26 @@ public function testWidget() {
   -    $assert_session->assertWaitOnAjaxRequest();
        // Assert the focus is set back on the open button of the media field.
        $this->assertJsCondition('jQuery("#field_twin_media-media-library-wrapper .js-media-library-open-button").is(":focus")');
   

   Perhaps we should change this to waiting for the expected element to have focus, instead of just removing the wait for AJAX?

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -590,38 +562,32 @@ public function testWidget() {
   -    $assert_session->assertWaitOnAjaxRequest();
        // Assert the focus is set back on the open button of the media field.
        $this->assertJsCondition('jQuery("#field_twin_media-media-library-wrapper .js-media-library-open-button").is(":focus")');
   

   Same here. Also, if we're going to wait for things to be focused, perhaps a helper method would be useful?

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -710,7 +671,6 @@ public function testWidget() {
   -    $assert_session->assertWaitOnAjaxRequest();
        // Assert the focus is set to the wrapper of the other selected item.
        $this->assertJsCondition('jQuery("#field_twin_media-media-library-wrapper .media-library-item").is(":focus")');
   

   Same here. I'll stop calling these out in the rest of this review.

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -722,14 +682,12 @@ public function testWidget() {
   -    $this->assertNotEmpty($assert_session->waitForText('Added one media item.'));
   -    $assert_session->assertWaitOnAjaxRequest();
   +    $this->waitForText('Added one media item.');
   

   I notice that previously, we were waiting for the text, then waiting for another AJAX request. Perhaps we need another wait condition after the waitForText()?

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1192,81 +1127,72 @@ public function testWidgetUpload() {
   -    $page->uncheckField("Select $existing_media_name");
   +    $selection_area->uncheckField("Select $existing_media_name");
   

   👍 More precise: I like that.

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1192,81 +1127,72 @@ public function testWidgetUpload() {
    +    $this->saveAnd('select');
    +    $this->waitForNoText('Save and select');
    +    $this->waitForText("Select $existing_media_name");
    

    This seems interesting -- why do we need the waitForNoText()?

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1394,30 +1312,29 @@ public function testWidgetOEmbed() {
    +    // consistently match their label's  "for" attribute.
    

    Supernit: there are two spaces after the word "label's". Thank you, by the way, for documenting the individual uses of assertWaitOnAjaxRequest().

12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1433,33 +1350,33 @@ public function testWidgetOEmbed() {
         // Assert we can only add supported URLs.
         $page->fillField('Add Type Five via URL', 'https://www.youtube.com/');
         $page->pressButton('Add');
    +    // assertWaitOnAjaxRequest() required for input "id" attributes to
    +    // consistently match their label's  "for" attribute.
    

    Same here.

13. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1433,33 +1350,33 @@ public function testWidgetOEmbed() {
         // Assert we can not add a video ID that doesn't exist. We need to use a
         // video ID that will not be filtered by the regex, because otherwise the
         // message 'No matching provider found.' will be returned.
         $page->fillField('Add Type Five via URL', 'https://www.youtube.com/watch?v=PWjcqE3QKBg1');
         $page->pressButton('Add');
    +    // assertWaitOnAjaxRequest() required for input "id" attributes to
    +    // consistently match their label's  "for" attribute.
    

    And here.

14. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1472,12 +1389,15 @@ public function testWidgetOEmbed() {
         // Assert we can add a oEmbed video with a custom name.
         $page->fillField('Add Type Five via URL', $youtube_url);
         $page->pressButton('Add');
    +    // assertWaitOnAjaxRequest() required for input "id" attributes to
    +    // consistently match their label's  "for" attribute.
    

    And here.

15. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1505,42 +1425,41 @@ public function testWidgetOEmbed() {
         $assert_session->hiddenFieldValueEquals('current_selection', $selected_item_id);
         $page->fillField('Add Type Five via URL', $youtube_url);
         $page->pressButton('Add');
    +    // assertWaitOnAjaxRequest() required for input "id" attributes to
    +    // consistently match their label's  "for" attribute.
    

    And here.

16. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1505,42 +1425,41 @@ public function testWidgetOEmbed() {
    +    $this->waitForText('The media item has been created but has not yet been saved');
         $page->fillField('Name', 'Another video');
    -    $selection_area = $assert_session->elementExists('css', '.media-library-add-form__selected-media');
    +
    +    $page->fillField('media[0][fields][name][0][value]', 'Another video');
    

    This is interesting; why the new $page->fillField() call, and why does it differ from the existing $page->fillField() call?

17. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $result = $page->waitFor($timeout / 1000, function () use ($page, $text) {
    

    Nit: No need to use ($page), it will get passed to the closure as an argument. (The callback given to waitFor() is always given the element that waitFor() was called on.)

18. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $assert_session = $this->assertSession();
    +    $result = $assert_session->waitForText($text, $timeout);
    

    Nit: $assert_session does not need to be its own variable.

19. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $assert_session = $this->assertSession();
    +    $element = $assert_session->waitForElement('css', "$selector:contains('$text')");
    

    Same here.

20. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $assert_session = $this->assertSession();
    +    $element = $assert_session->waitForElement($selector, $locator, $timeout);
    

    And here.

21. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +  protected function clickTypeTab($type) {
    

    I would maybe rename this method to something like switchToMediaType or similar.

22. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    // consistently match their label's  "for" attribute.
    

    Two spaces after "label's".

23. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $assert_session->assertWaitOnAjaxRequest();
    

    $assert_session doesn't need to be its own variable here.

24. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $start = microtime(TRUE);
    +    $end = $start + ($timeout / 1000);
    +    do {
    +      $nodes = $page->findAll($selector_type, $selector);
    +      if (count($nodes) === $count) {
    +        return;
    +      }
    +      usleep(100000);
    +    } while (microtime(TRUE) < $end);
    

    Why not use $page->waitFor() here?

25. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $assert_session->elementsCount($selector_type, $selector, $count);
    

    No need for $assert_session variable.

26. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    $assert_session = $this->assertSession();
    +    $page = $this->getSession()->getPage();
    +
    +    $start = microtime(TRUE);
    +    $end = $start + ($timeout / 1000);
    +    do {
    +      $node = $page->findField($field);
    +      if (!is_null($node)) {
    +        return $node;
    +      }
    +      usleep(100000);
    +    } while (microtime(TRUE) < $end);
    +
    +    return $assert_session->fieldExists($field);
    

    Couldn't we just use $assert_session->waitForField(), then return $assert_session->fieldExists()?

27. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +  /**
    +   * Checks if a field exists and provides it a value.
    +   *
    +   * @param string $field
    +   *   The field to find.
    +   * @param string $value
    +   *   The value entered in the field.
    +   * @param int $timeout
    +   *   Timeout in milliseconds, defaults to 10000.
    +   *
    +   * @todo replace with whatever gets added in
    +   *   https://www.drupal.org/node/3061852
    +   */
    +  protected function waitForFillField($field, $value, $timeout = 10000) {
    +    if ($field = $this->waitForFieldExists($field, $timeout)) {
    +      $field->setValue($value);
    +    }
    +  }
    

    This seems a bit superfluous. Couldn't we just do $this->waitForFieldExists()->setValue() in the test?

28. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +  /**
    +   * Clicks "Save and select||insert" button and waits for AJAX completion.
    +   *
    +   * @param string $operation
    +   *   The final word of the button to be clicked.
    +   */
    +  protected function saveAnd($operation) {
    

    I saw a call, elsewhere in the test, to $this->saveAs('select', FALSE)...but this method doesn't take a second argument. 🤔

29. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +    // consistently match their label's  "for" attribute.
    

    Two spaces after "label's".

30. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1614,13 +1527,238 @@ public function testFieldUiIntegration() {
    +  protected function mediaLibraryOpenButton($button_name, $after_open_selector = '.media-library-menu') {
    

    I would maybe rename this to openMediaLibraryForField().

Re #46

1. ✅ done
2. ✅ 😁
3. ✅ 😁
4. ✅ No followup needed, comment was outdated so I removed.
5. ✅ assertJsCondition() already uses a wait, so the preceding assertWaitOnAjaxRequest() is unnecessary
6. ✅ assertJsCondition() already uses a wait, so the preceding assertWaitOnAjaxRequest() is unnecessary
7. ✅ assertJsCondition() already uses a wait, so the preceding assertWaitOnAjaxRequest() is unnecessary
8. ✅ Yep, probably good to not just rely on messages to confirm something is completed, changed the elementContains() that follows to a waitForElementContains()
9. ✅ 😁
10. ✅ The waitForNoText() looks like it's unnecessary , that's likely an artifact of huge patch.
11. ✅ 👾
12. ✅ 👾
13. ✅ 👾
14. ✅ 👾
15. ✅ 👾

16. This is interesting; why the new $page->fillField() call, and why does it differ from the existing $page->fillField() call?

    Ah, that was back when I thought the input/label id/for issue was specific to just that field (it is where it most often failed). That can return to its original form now that the unfortunately-needed assertWaitOnAjaxRequest() calls are back

17. ✅
18. ✅
19. ✅
20. ✅
21. ✅
22. ✅👾
23. ✅
24. ✅
25. ✅
26. ✅Yep that's all that is needed. Probably a symptom of banging my head against the wall.
27. ✅Yep, see previous item.
28. ✅
    

    I saw a call, elsewhere in the test, to $this->saveAs('select', FALSE)...but this method doesn't take a second argument.

    Good catch, that is from a troubleshooting version of the patch with extra DB checking to after media is added to the database 3055648-45-WITH-DATABASE-CHECKS Not needed here so the arg is removed.

29. ✅ 👾

Didn't get all the way through the patch, but a closer read made me notice some more things about which I have small, easily-swatted-away questions.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
+  protected function waitForText($text, $timeout = 10000) {
+    $result = $this->assertSession()->waitForText($text, $timeout);
+    $this->assertNotEmpty($result, "\"$text\" not found");
+  }

Stuff like this should really be part of WebDriverTestBase. I think we should open an issue for it.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -439,23 +433,19 @@ public function testWidget() {
   +    $this->openMediaLibraryForField('field_unlimited_media');
   +    $this->waitForText('Add or select media');
   

   I think that this waitForText() assertion should be part of openMediaLibraryForField(). I can't think of any circumstance where we wouldn't want to assert the presence of that text as part of opening the media library.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -439,23 +433,19 @@ public function testWidget() {
   +    $menu = $this->openMediaLibraryForField('field_unlimited_media');
   

   I think it's a little awkward that openMediaLibraryForField() returns the menu by default. I think we should just not have a default value for that parameter (or at least default it to NULL), and make explicit the fact that we want to retrieve the menu once the media library is opened. IOW, this should be $this->openMediaLibraryForField('field_unlimited_media', '.media-library-menu'). Calling $this->openMediaLibraryForField('field_unlimited_media') should not return any element, IMHO.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -463,23 +453,18 @@ public function testWidget() {
        $assert_session->elementExists('css', '.ui-dialog-titlebar-close')->click();
   -    $assert_session->assertWaitOnAjaxRequest();
   

   Nice call on removing this assertWaitOnAjaxRequest() call. There is no reason to wait for AJAX after closing the modal; no AJAX request is triggered by that, as far as I know.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -489,9 +474,9 @@ public function testWidget() {
   +    $this->waitForElementTextContains('.media-library-selected-count', '0 of 1 item selected');
   

   IMHO, waitForElementTextContains() should follow similar patterns to what we get from Mink. In other words, it shouldn't force you to use a CSS selector; it should be something like $this->waitForElementTextContains('css', '.media-library-selected-count', '0 of 1 item selected').

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -514,18 +498,15 @@ public function testWidget() {
   -    $assert_session->buttonExists('Show row weights')->press();
   +    $this->assertElementExistsAfterWait('css', '#field-twin-media .tabledrag-toggle-weight')->press();
   

   This seems interesting; why the change from a button value to a CSS selector?

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -690,8 +651,7 @@ public function testWidget() {
   +    $this->switchToMediaType('One');
   

   I don't know how I feel about the "magic" of $this->switchToMediaType('One'), when the actual name of the media type is "Type One". I'd rather this (and all similar calls) was more explicit, i.e. $this->switchToMediaType('Type One').

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -699,9 +659,8 @@ public function testWidget() {
   +    $open_button = $this->assertElementExistsAfterWait('css', '.media-library-open-button[name^="field_twin_media"]');
        $this->assertTrue($open_button->hasAttribute('data-disabled-focus'));
        $this->assertTrue($open_button->hasAttribute('disabled'));
   

   These can be collapsed into one line:

   $this->assertElementExistsAfterWait('css', '.media-library-open-button[name^="field_twin_media"][data-disabled-focus][disabled]').

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -972,18 +919,16 @@ public function testWidgetAnonymous() {
        $this->assertNotEmpty($assert_session->waitForText('Added one media item.'));
   -    $assert_session->assertWaitOnAjaxRequest();
   

   Why didn't this change to $this->waitForText()?

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -997,6 +942,9 @@ public function testWidgetAnonymous() {
   +   * Note that this test will occasionally fail with SQLite until
   +   * https://www.drupal.org/node/3066447 is addressed.
   

   This should be a @todo.

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1166,13 +1099,13 @@ public function testWidgetUpload() {
    -    $page->attachFileToField('Add files', $this->container->get('file_system')->realpath($png_uri_3));
    -    $assert_session->assertWaitOnAjaxRequest();
    +    $this->addMediaFileToField('Add files', $this->container->get('file_system')->realpath($png_uri_3));
    +    $this->waitForText('The media item has been created but has not yet been saved.');
         $assert_session->checkboxChecked("Select $existing_media_name");
    

    This looks like a change in logic. Wouldn't we want to wait for the "Select $existing_media_name" field instead, to keep it in line with what it previously was?

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1200,81 +1133,74 @@ public function testWidgetUpload() {
    +    $this->waitForText("Select $existing_media_name");
    

    Isn't this the name of a field? If so, wouldn't we want to wait for the field to exist, rather than some text? This feels a bit "coincidental" otherwise.

12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1472,33 +1389,33 @@ public function testWidgetOEmbed() {
    +    // assertWaitOnAjaxRequest() required for input "id" attributes to
    +    // consistently match their label's "for" attribute.
         $assert_session->assertWaitOnAjaxRequest();
    

    So, question about this. If we are waiting for a specific condition (the input 'id' to match the label 'for'), couldn't we use $this->assertJsCondition() or similar for that? Do we need to use assertWaitOnAjaxRequest()?

Also, question about the naming of assertElementExistsAfterWait() -- that seems to be a rather verbose name. Why not call it waitForElement(), for consistency with the other "wait" methods in this test?

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   -    $assert_session->assertWaitOnAjaxRequest();
   +    $this->assertElementExistsAfterWait('css', '[name="settings[handler_settings][target_bundles][type_one]"][checked="checked"]');
        $page->checkField('settings[handler_settings][target_bundles][type_two]');
   -    $assert_session->assertWaitOnAjaxRequest();
   +    $this->assertElementExistsAfterWait('css', '[name="settings[handler_settings][target_bundles][type_two]"][checked="checked"]');
        $page->checkField('settings[handler_settings][target_bundles][type_three]');
   -    $assert_session->assertWaitOnAjaxRequest();
   +    $this->assertElementExistsAfterWait('css', '[name="settings[handler_settings][target_bundles][type_three]"][checked="checked"]');
   

   Nit: I think we can change [checked="checked"] to just [checked]. That is a boolean attribute, so its value doesn't really matter. :)

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +  protected function assertElementExistsAfterWait($selector, $locator, $timeout = 10000) {
   

   I think we should rename this to waitForElement(), which IMHO would be clearer and less verbose.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +    $lowercase_type = strtolower($type);
   

   This should be mb_strtolower().

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +    $this->assertElementExistsAfterWait('css', "[data-drupal-media-type='type_$lowercase_type']");
   

   I'm not entirely clear on why we need this -- what is it adding? Can we get a comment? above it?

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +    // assertWaitOnAjaxRequest() required for input "id" attributes to
   +    // consistently match their label's "for" attribute.
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   

   Ah, now I see why we can't wait for a particular condition instead of doing this: because we're waiting for all input IDs to match the 'for' attributes. Yeah, that'd be pretty impossible to "wait" for. I guess assertWaitOnAjaxRequest() is our only option. For now.

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +   * @param string $selector_type
   +   *   Element selector type (css, xpath)
   +   * @param string|array $selector
   +   *   Element selector.
   

   These should be renamed $selector and $locator for consistency with Mink.

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +    $page = $this->getSession()->getPage();
   +
   +    $start = microtime(TRUE);
   +    $end = $start + ($timeout / 1000);
   +    do {
   +      $nodes = $page->findAll($selector_type, $selector);
   +      if (count($nodes) === $count) {
   +        return;
   +      }
   +      usleep(100000);
   +    } while (microtime(TRUE) < $end);
   +
   +    $this->assertSession()->elementsCount($selector_type, $selector, $count);
   

   I'm still not clear on why we don't use $page->waitFor() here? Something like:

   $result = $this->getSession()->getPage()->waitFor($timeout / 1000, function ($page) use ($selector, $locator, $count) {
     return count($page->findAll($selector, $locator)) === $count;
   });
   $this->assertSame($result, $count);
   

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +  protected function waitForFieldExists($field, $timeout = 10000) {
   

   Can we rename this waitForField(), for consistency?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +  /**
   +   * Waits for a file field to exist before uploading.
   +   */
   +  public function addMediaFileToField($locator, $path) {
   +    $page = $this->getSession()->getPage();
   +    $this->waitForFieldExists($locator);
   +    $page->attachFileToField($locator, $path);
   +  }
   

   This can be refactored: $this->waitForFieldExists($locator)->attachFile(). No need to use $page. Honestly, since it's so simple, we might be able to just remove this method entirely.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1653,13 +1564,201 @@ public function testFieldUiIntegration() {
   +  protected function saveAnd($operation) {
   +    $this->assertElementExistsAfterWait('css', '.ui-dialog-buttonpane')->pressButton("Save and $operation");
   

   I'm a bit iffy on the magic string concatenation we're doing, but...it's probably fine for now. This is not, after all, an API.

I discussed this with @effulgentsia, @oknate, and @webchick in a scrum call today.

Here's the thing: this is blocking #3034242: Hide "Save and insert" and "Additional selected media" from users by default, which is a feature that we really need to land before alpha. @oknate brilliantly pointed out that this patch, in its current form, does exactly what it sets out to do -- namely, significantly reduce random failures in MediaLibraryTest. The changes I want are, compared to that, minor refactoring.

So, to unblock the feature, let's get this in now, as is, and everyone can breathe a sigh of relief and that feature can get in as soon as humanly possible. Then we'll open a follow-up to do further clean-up in MediaLibraryTest, which as I understand it can be done all the way through beta.

#47 is already green on all backends (the failure on SQLite is a known thing which has a separate issue, and is referenced in the patch), so this is ready to go now.

This addresses all the points from #48 & #49 with a few exceptions:

#48.8 most existing uses of $this->assertNotEmpty($assert_session->waitForText('foo')); were not changed. This can definitely be addressed in a followup.

#49.7 (first) will look into refactoring in a followup, the suggested approach simply didn't occur to me.
#49.2 (second) keeping addMediaFileToField() as I had to add assertWaitOnAjaxRequest() for the same for/id issue. Otherwise - intermittently - the alt text field couldn't be found.

This is just a re-upload of #47, the version that was RTBC'd. Will open a followup to address the feedback received after that, so this can get in and stop these test failures.

Great idea. RTBC once green-ish. (Failure in the SQLite run of the x90 patch is expected.)

Followup: #3087227: [META] Split up and refactor MediaLibraryTest

Micronit (nanonit?)

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -1472,33 +1389,33 @@ public function testWidgetOEmbed() {
+    // assertWaitOnAjaxRequest() required for input "id" attributes to
+    // consistently match their label's "for" attribute.
...
+    // consistently match their label's "for" attribute.

@@ -1511,12 +1428,15 @@ public function testWidgetOEmbed() {
+    // assertWaitOnAjaxRequest() required for input "id" attributes to

@@ -1544,42 +1464,39 @@ public function testWidgetOEmbed() {
+    // assertWaitOnAjaxRequest() required for input "id" attributes to

I don't see assertWaitOnAjaxRequest being used here anymore? 🤔

As just comment changes, fine to go straight back to RTBC

sorry, my eyes don't work today...i didn't read the untouched lines of the patch properly

Yup. I just applied the patch locally and confirmed that each call to assertWaitOnStupidUnreliableBS() -- I mean, assertWaitOnAjaxRequest() -- is accompanied by a comment. That was done at my request, so we'd know explicitly where and why we relied on an unreliable method. :)

Updating credit

Committed and pushed 312ce8ecb3 to 9.0.x and a721bed4c0 to 8.9.x and bef49c3af9 to 8.8.x. Thanks!

Unfortunately this test is still failing quite a lot - maybe even more - for example https://www.drupal.org/pift-ci-job/1433935