[META] Split up and refactor MediaLibraryTest

Problem/Motivation

MediaLibraryTest is, quite frankly, a mess.

Its coverage is very extensive, but it is also extremely long and complicated, with a lot of repeated code, a few somewhat unreliable parts (though this has significantly improved after #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest), and a bloated, unclear scope. It is probably the single most important test in Media Library, but it's not particularly maintainable.

Proposed resolution

First, split MediaLibraryTest into several smaller tests, based on a single base class with common assertions and helpers. For this first step, we won't refactor the tests' business logic; we'll just split them up. They'll be messy, but at least they'll be of a more manageable size: #3095210: Split up MediaLibraryTest

Then, implement various changes that were suggested in other issues related to MediaLibraryTests. Some of these may no longer be applicable due to other changes, but there are many good suggestions in there that are still relevant. These things should be done in sparate issues covering individual classes derived from the big split-up of MediaLibraryTest.

From seanB in #3088681: Make MediaLibraryTest less dependent on representational CSS selectors

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

   Maybe it makes sense to still assert the modal title as a generic library UI element? Or add this assert to openMediaLibraryForField?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -462,8 +461,8 @@ public function testWidget() {
   -    $assert_session->elementNotExists('css', '.media-library-view .media-library-item__edit');
   ...
   +    $assert_session->elementNotExists('css', '.view-media-library .media-library-item__edit');
   +    $assert_session->elementNotExists('css', '.view-media-library .media-library-item__remove');
   

   media-library-item__edit and media-library-item__remove are in the view, so I guess that is fine to keep those.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -479,22 +478,20 @@ public function testWidget() {
   +    $this->openMediaLibraryForField('field_single_media_type', '#media-library-wrapper');
   

   Can't we fix the openMediaLibraryForField to use a different default $after_open_selector instead of passing the wrapper ID? Or maybe always use the wrapper ID as default? I guess waiting for the media library view makes most sense since that is always going to be there.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1069,13 +1034,11 @@ public function testWidgetUpload() {
   -    $assert_session->elementExists('css', '.media-library-item__remove')->click();
   +    $assert_session->elementExists('css', '.field--name-field-twin-media')->pressButton('Remove');
   

   We are using .media-library-item__remove earlier in the test since that comes from the view? Why remove it here?

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2228,21 +2116,35 @@ protected function assertElementExistsAfterWait($selector, $locator, $timeout =
   +  protected function getTypesMenu() {
   

   Do we really need an extra method for this?

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2351,17 +2253,170 @@ protected function pressSaveButton($expect_errors = FALSE) {
   +  protected function openMediaLibraryForField($field_name, $after_open_selector = '.js-media-library-menu') {
   ...
        return $this->assertElementExistsAfterWait('css', $after_open_selector);
   

   We assert the media library is closed by $this->waitForNoText('Add or select media');. Not sure if we should do the reverse here and assert the title does exist?

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2351,17 +2253,170 @@ protected function pressSaveButton($expect_errors = FALSE) {
   +    $assert_session->pageTextMatches('/The media items? ha(s|ve) been created but ha(s|ve) not yet been saved. Fill in any required fields and save to add (it|them) to the media library./');
   

   Do we want have a more specific assert for this? We could technically show the wrong message and it would still pass?

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2351,17 +2253,170 @@ protected function pressSaveButton($expect_errors = FALSE) {
   +  protected function insertSelected($expected_announcement = NULL) {
   ...
   +  protected function getCheckboxes() {
   ...
   +  protected function selectItem($index, $expected_selected_count = NULL) {
   

   These look like method names that are so generic that they might lead to issues in the future if a parent decides to implement them. Maybe make it more media specific?

From @phenaproxima in #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest:

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()?

From @phenaproxima in #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

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);
   

8. +++ 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?

9. +++ 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.

10. +++ 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.

Remaining tasks

All of it.

User interface changes

None whatsoever.

API changes

MediaLibraryTest is not an API. So, although it will change extensively, there are no API changes.

Data model changes

Certainly not.

Release notes snippet

None expected.