Convert all web tests to BTB where possible, increase test coverage and split existing tests into more granular classes.

This would make a lot of sense. We can use this ticket to plan and than create patches for each step in sub-ticket (as @Sam152 already proposed).

working on this

WIP

Still missing:
- MediaViewsWizardTest
- MediaViewsBulkFormTest
- Any other new test we want to implement to increase coverage (for instance IEF integration, etc)

will be working on this soon

All existing web tests were converted, and additional tests were created for the IEF integration.

The resulting test classes are:

- (MediaEntityJavascriptTestBase)
- BasicCreationTest
- MediaUiTest
- MediaIefIntegrationTest
- MediaViewsWizardTest
- MediaBulkFormTest

Do we want anything else to increase coverage?

All tests pass locally... anyone has any suggestion on what I should look for to try to fix the failure with the testbot?

Thanks!

Thanks for the tip @Sam152 !

Reviewing the diff I realized that for some reason (probably copy/paste fail) I had left out a good chunk of the previous MediaUITest, sorry about that. Now it includes everything.

There was some duplicated code in MediaUITest (covered also in the ViewsWizardTest), and also this test is quite big, we could probably split it into smaller ones, but I left everything in a single file to make reviewing of the patch (comparing with the previous test) easier.

Re-rolled and chased changes in 2806611-12

Created a PR for the same patch in #14 for easier review on github:
https://github.com/drupal-media/media_entity/pull/81

Great work @marcoscano.

Patch is huge and i tried to focus on main functionality and compare test methods with original test methods, just to make sure we don't miss something.
So, here we are:

1. +++ /dev/null
   @@ -1,193 +0,0 @@
   -  public function testMediaAccess() {
   

   This test method from old test is merged somewhere or we miss him?

2. +++ /dev/null
   @@ -1,511 +0,0 @@
   -  public function testMediaViewsWizard() {
   

   Same here. I don't see this method in new tests.

3. +++ /dev/null
   @@ -1,511 +0,0 @@
   -    $this->drupalGet('admin/content/media', ['query' => ['provider' => $second_media_bundle['id']]]);
   -    $this->assertResponse(200);
   -    $this->assertNoText($first_media_item['name[0][value]']);
   -    $this->assertText($second_media_item['name[0][value]']);
   -    $this->assertText($second_media_bundle['label']);
   

   We missing filter for second bundle in new test. Yes it's same as the first, but we need to keep him i think.

4. +++ b/tests/src/FunctionalJavascript/MediaBulkFormTest.php
   @@ -0,0 +1,134 @@
   +      $this->assertFalse($this->loadMedia($i)->isPublished(), 'The unpublish action failed in some of the media entities.');
   ...
   +      $this->assertTrue($this->loadMedia($i)->isPublished(), 'The publish action failed in some of the media entities.');
   ...
   +      $this->assertNull($this->loadMedia($i), 'Could not delete some of the media entities.');
   

   Why we changed those messages? :) Original messages are not clear?

Thanks @CTaPByK for reviewing!

1. This test method from old test is merged somewhere or we miss him?

Another copy/paste fail! :( Sorry about that. Corrected in the attached patch.

2. Same here. I don't see this method in new tests.

This code duplicated some of the code in the old WizardTest.php, so there is only one test after the conversion.

We missing filter for second bundle in new test. Yes it's same as the first, but we need to keep him i think.

I'm not sure to completely understand your feedback here. The new version of the mentioned code is testing whether the filtered value is present (first item), and that the second value (filtered out) is not present in the page. What else should we test?

4. Why we changed those messages? :) Original messages are not clear?

In phpunit, differently from simpletest assertion messages, the optional messages that most asserts accept will be printed in case of failure, not on success. That's why we need to update the message strings, at least to negate the sentence.

Just for reference, the current test classes after the conversion are:

- (MediaEntityJavascriptTestBase)
- BasicCreationTest
- MediaAccessTest
- MediaUiTest
- MediaIefIntegrationTest
- MediaViewsWizardTest
- MediaBulkFormTest

interdiffed correctly but messed up generating the actual patch... #facepalm

Just one nutpick:

+++ b/tests/src/FunctionalJavascript/MediaAccessTest.php
@@ -0,0 +1,104 @@
+    $session = $this->getSession();
+    $page = $session->getPage();

Unused variables.

Other then that, looks great.

Great work! Just a few suggestions for performance improvements and things related to plans for core:

1. +++ b/tests/src/FunctionalJavascript/BasicCreationTest.php
   @@ -0,0 +1,79 @@
   +class BasicCreationTest extends MediaEntityJavascriptTestBase {
   

   This could be a kernel test, which would speed it up significantly.

2. +++ b/tests/src/FunctionalJavascript/MediaAccessTest.php
   @@ -0,0 +1,104 @@
   +class MediaAccessTest extends MediaEntityJavascriptTestBase {
   

   This can be browser test. It doesn't need JS it seems.

3. +++ b/tests/src/FunctionalJavascript/MediaBulkFormTest.php
   @@ -0,0 +1,134 @@
   +class MediaBulkFormTest extends MediaEntityJavascriptTestBase {
   

   This could be BrowserTest as well.

4. +++ b/tests/src/FunctionalJavascript/MediaBulkFormTest.php
   @@ -0,0 +1,134 @@
   +  /**
   +   * Load the specified media from the storage.
   +   *
   +   * @param int $id
   +   *   The media identifier.
   +   *
   +   * @return \Drupal\media_entity\MediaInterface
   +   *   The loaded media entity.
   +   */
   +  protected function loadMedia($id) {
   +    /** @var \Drupal\media_entity\MediaStorage $storage */
   +    $storage = $this->container->get('entity_type.manager')->getStorage('media');
   +    return $storage->loadUnchanged($id);
   +  }
   

   Let's add storage as a class member. Then this becomes $this->storage->loadUnchanged() and we can remove the helper function.

5. +++ b/tests/src/FunctionalJavascript/MediaEntityJavascriptTestBase.php
   @@ -0,0 +1,163 @@
   +  /**
   +   * Waits for jQuery to become ready and animations to complete.
   +   */
   +  protected function waitForAjaxToFinish() {
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   +  }
   

   Since this wraps a one-liner lets just remove it.

6. +++ b/tests/src/FunctionalJavascript/MediaEntityJavascriptTestBase.php
   @@ -0,0 +1,163 @@
   +  /**
   +   * Debugger method to save additional HTML output.
   +   *
   +   * The base class will only save browser output when accessing page using
   +   * ::drupalGet and providing a printer class to PHPUnit. This method
   +   * is intended for developers to help debug browser test failures and capture
   +   * more verbose output.
   +   */
   +  protected function saveHtmlOutput() {
   +    $out = $this->getSession()->getPage()->getContent();
   +    // Ensure that any changes to variables in the other thread are picked up.
   +    $this->refreshVariables();
   +    if ($this->htmlOutputEnabled) {
   +      $html_output = '<hr />Ending URL: ' . $this->getSession()->getCurrentUrl();
   +      $html_output .= '<hr />' . $out;
   +      $html_output .= $this->getHtmlOutputHeaders();
   +      $this->htmlOutput($html_output);
   +    }
   +  }
   

   Taking plans to move ME to core into consideraton I think that we need to remove this.

7. +++ b/tests/src/FunctionalJavascript/MediaIefIntegrationTest.php
   @@ -0,0 +1,113 @@
   +    /** @var \Drupal\media_entity\MediaBundleInterface $media_bundle */
   +    $media_bundle = $this->drupalCreateMediaBundle();
   +
   +    // Create a new content type.
   +    $values = [
   +      'name' => 'Media entity CT',
   +      'title_label' => 'An example Custom Content type.',
   +      'type' => 'media_entity_ct',
   +    ];
   +    $content_type = $this->createContentType($values);
   +    // Create an entity_reference field.
   +    FieldStorageConfig::create([
   +      'field_name' => 'ref_media_entities',
   +      'type' => 'entity_reference',
   +      'entity_type' => 'node',
   +      'cardinality' => FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED,
   +      'settings' => [
   +        'target_type' => 'media',
   +      ],
   +    ])->save();
   +    FieldConfig::create([
   +      'field_name' => 'ref_media_entities',
   +      'field_type' => 'entity_reference',
   +      'entity_type' => 'node',
   +      'bundle' => $content_type->id(),
   +      'label' => 'Media referenced',
   +      'settings' => [
   +        'handler' => 'default:media',
   +        'handler_settings' => [
   +          'target_bundles' => [
   +            $media_bundle->id() => $media_bundle->id(),
   +          ],
   +          'sort' => [
   +            'field' => '_none',
   +          ],
   +          'auto_create' => FALSE,
   +          'auto_create_bundle' => $media_bundle->id(),
   +        ],
   +      ],
   +    ])->save();
   +
   +    // Set widget to inline_entity_form.
   +    /** @var \Drupal\Core\Entity\Display\EntityFormDisplayInterface $form_display */
   +    $form_display = $this->container->get('entity_type.manager')
   +      ->getStorage('entity_form_display')
   +      ->load('node.media_entity_ct.default');
   +    $form_display->setComponent('ref_media_entities', [
   +      'type' => 'inline_entity_form_complex',
   +      'settings' => [],
   +    ])->save();
   

   Let's move this to setUp().

8. +++ b/tests/src/FunctionalJavascript/MediaUiTest.php
   @@ -0,0 +1,346 @@
   +  public function testMediaWithOnlyOneBundle() {
   ...
   +  public function testMediaWithMultipleBundles() {
   

   I think that this two don't need JS. Since the other test function in this class does it would make sense to split and move this two into a BrowserTest.

Thanks for reviewing!

It is true that after re-organizing the tests in Kernel/Browser/Javascript, some of the previous code distribution changed a bit (base classes, traits, etc). I hope the result makes sense, more or less...

The final structure of the test classes after the re-organization is:

Kernel
--BasicCreationTest
--TokensTest
Functional
--MediaEntityFunctionalTestTrait
--MediaEntityFunctionalTestBase
--MediaAccessTest
--MediaBulkFormTest
--MediaUiFunctionalTest
FunctionalJavascript
--MediaEntityJavascriptTestBase
--MediaUiJavascriptTest
--MediaIefIntegrationTest
--MediaViewsWizardTest

Committed. Thanks!

Noticed a nit.