Add helper methods to the migrate_drupal_ui functional tests

I started looking at this patch. Here are some initial suggestions. I may have more when I re-review.

1. Let me add some lines of context to what I see in the patch:

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
    @@ -11,13 +11,6 @@ abstract class MigrateUpgradeExecuteTestBase extends MigrateUpgradeTestBase {
    ...
    -  /**
    -   * The destination site major version.
    -   *
    -   * @var string
    -   */
    -  protected $destinationSiteVersion;
   
       /**
        * {@inheritdoc}
        */
       protected function setUp() {
         parent::setUp();
   
         // Create content.
         $this->createContent();
   
         // Get the current major version.
         [$this->destinationSiteVersion] = explode('.', \Drupal::VERSION, 2);
       }
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -31,6 +32,13 @@ abstract class MigrateUpgradeTestBase extends BrowserTestBase {
        */
       protected $sourceDatabase;
   
    +  /**
    +   * The destination site major version.
    +   *
    +   * @var string
    +   */
    +  protected $destinationSiteVersion;

   Without looking at it too closely: if we are moving this property to the parent class, should we also move the lines in setUp() that set its value?

2.   diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
     @@ -165,39 +173,19 @@ protected function assertUpgradePaths(WebAssert $session, array $available_paths
     -  protected function assertIdConflict(WebAssert $session, array $entity_types) {
     -    /** @var \Drupal\ $entity_type_manager */
     +  protected function assertIdConflictForm(WebAssert $session, array $entity_types) {
     +    /** @var \Drupal\Core\Entity\EntityTypeManager $entity_type_manager */

   Search for "assertIdConflict". It is used in an assertion message in this file. I have not checked other files.

3.   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
     @@ -260,4 +248,83 @@ protected function assertMigrationResults(array $expected_counts, $version) {
     +   * @throws \Behat\Mink\Exception\ExpectationException
     +   */
     +  protected function assertReviewForm(WebAssert $session, array $available_paths, array $missing_paths) {
     +    $this->assertSession()->pageTextContains('What will be upgraded?');
     +    $this->assertUpgradePaths($session, $available_paths, $missing_paths);
     +  }

   Do we need an @throw annotation when an exception might be thrown indirectly (i.e., from a function that this function calls)? Do we need this function at all, or can we add the additional assertion to assertUpgradePaths()? (I have not checked.) If we do need this function, then why not use $session instead of $this->assertSession()?

4.   diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
     @@ -165,39 +173,19 @@ protected function assertUpgradePaths(WebAssert $session, array $available_paths
     ...
     -  protected function assertReviewPage(WebAssert $session, array $available_paths, array $missing_paths) {
     -    $this->assertText('What will be upgraded?');
     -
     -    // Ensure there are no errors about the missing modules from the test module.
     -    $session->pageTextNotContains(t('Source module not found for migration_provider_no_annotation.'));
     -    $session->pageTextNotContains(t('Source module not found for migration_provider_test.'));
     -    $session->pageTextNotContains(t('Destination module not found for migration_provider_test'));

   It looks as though that last assertion is not replaced.

Thanks for making those changes. I have a few more suggestions.

1.   diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
     @@ -260,4 +251,85 @@ protected function assertMigrationResults(array $expected_counts, $version) {
     +  protected function assertReviewForm(WebAssert $session, array $available_paths, array $missing_paths) {
     +    $session->pageTextContains('What will be upgraded?');
     +    $this->assertUpgradePaths($session, $available_paths, $missing_paths);
     +  }

   Do we need two helper functions, where one does a single assertion and then calls the other? In other words, is there any harm in asserting "What will be upgraded?" every time we call assertUpgradePaths()?

   If the answers are "no", then I would like to add the assertion to assertUpgradePaths() and then rename it to assertReviewForm().

2. Related: almost every time that assertUpgradePaths() is called, it looks like this:

    $available_paths = $this->getAvailablePaths();
    $missing_paths = $this->getMissingPaths();
    $this->assertUpgradePaths($session, $available_paths, $missing_paths);

   Can we make the two array arguments optional, using those methods as defaults? Maybe this change is out of scope for this issue, but if we do (1) then I think we can call it in scope.

3.   diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
     @@ -260,4 +251,85 @@ protected function assertMigrationResults(array $expected_counts, $version) {
     +  /**
     +   * Helper to assert content on the Overview form.
     +   *
     +   * @param \Drupal\Tests\WebAssert $session
     +   *   The web-assert session.
     +   *
     +   * @throws \Behat\Mink\Exception\ExpectationException
     +   */
     +  protected function assertOverviewForm(WebAssert $session) {
     +    $session->responseContains("Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal $this->destinationSiteVersion.");
     +  }
     +
     +  /**
     +   * Helper to assert content on the Overview form.
     +   *
     +   * @param \Drupal\Tests\WebAssert $session
     +   *   The web-assert session.
     +   *
     +   * @throws \Behat\Mink\Exception\ExpectationException
     +   */
     +  protected function assertIncrementalForm(WebAssert $session) {
     +    $session->pageTextContains("An upgrade has already been performed on this site. To perform a new migration, create a clean and empty new install of Drupal $this->destinationSiteVersion. Rollbacks are not yet supported through the user interface.");
     +  }

   The doc blocks are identical. The second function is called only once; like the first function, it is called immediately after $this->drupalGet('/upgrade');. The name of the second function is problematic, since it suggests that there is an Incremental form.

   I suggest that we solve all of these annoyances by adding an optional boolean parameter, perhaps $is_incremental, to the first of these functions and eliminate the second function. I think we can always test for "Upgrade a site …" and conditionally test for the second string.

   Is there a reason to use responseContains() instead of pageTextContains()? For HTML responses, they are equivalent, right?

4.   +  /**
     +   * Helper method to assert the text on the 'Upgrade analysis report' page.
     ...
     +   */
     +  protected function assertReviewForm(WebAssert $session, array $available_paths, array $missing_paths) {

   Is there any reason to vary from the pattern of the other short descriptions?

5.   +  protected function assertUpgrade($session, $version, array $entity_counts) {
     +    $session->pageTextContains(t('Congratulations, you upgraded Drupal!'));
     +    $this->assertMigrationResults($entity_counts, $version);
     +  }

   Again, do we need both methods? I searched for assertMigrationResults(), and it is not called except for here. Why not just add the assertion to that function?

Follow-up to #7.2: After applying the patch in #8, I still see the line

    $this->assertNotEmpty($entity_types, 'No entity types provided to \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase::assertIdConflict()');

in MigrateUpgradeTestBase.

Thanks, the patch in #12 looks great! RTBC

I rewrote the issue summary using the standard template, in order to help the final review. The last couple of points might be considered out of scope; I made the issue title a little more generic in the hope that at least assertUserLogIn() will be considered in scope. If we want to avoid objections to the scope, we might preemptively revert the move of $destinationSiteVersion until we remove MigrateUpgradeExecuteTestBase.

Oops, I forgot to update the status.

Two unrelated tests are failing. (At least, I think they are unrelated.) I will ask the testbot to try again.

Thanks for the reroll! I see just one problem:

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NodeClassicTest.php
    @@ -82,23 +82,23 @@ public function testMigrateUpgradeExecute() {
         $this->drupalGet('/upgrade');
    +
         $session = $this->assertSession();
    -    $session->responseContains("Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal $this->destinationSiteVersion.");
    +    $this->drupalGet('/upgrade');
    +    $this->assertOverviewForm($session);

The patch in #12 removed the first drupalGet() line. After the patch in #20, that function is called twice.

This reroll makes me think I was right to put off reviewing the other children of #2974445: [Meta] Refactor Migrate Drupal UI Functional tests until the first two are committed.

Nice work. Back to RTBC.

I compared (diff, not interdiff) the patches in #12 and #26. Aside from routine stuff (different line numbers and commit hashes, changes to context lines, etc.) the main difference was to change span to td in CSS selectors: moved from assertUpgradePaths() to assertReviewForm(). As you say, that change comes from #3134300: Simplify ReviewForm::buildForm() and the resulting markup.

I then re-reviewed the patch as a whole, looking for reroll problems like the one I pointed out in #21. To make the review of MigrateUpgradeTestBase easier, I moved assertReviewForm() before getSourceBasePath() so that I could compare it to assertUpgradePaths().

I did not remember why the assertions in assertReviewPage() were not added to one of the new helper functions, but I see that we already discussed one of them in #7.4, and I see the other pageTextNotContains() lines are still repeated in MigrateUpgradeExecuteTestBase.

Back to RTBC.

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -212,21 +175,101 @@ protected function assertIdConflict(WebAssert $session, array $entity_types) {
   +  protected function assertOverviewForm(WebAssert $session) {
   +    $session->responseContains("Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal $this->destinationSiteVersion.");
   +  }
   ...
   +  protected function assertIncrementalForm(WebAssert $session) {
   +    $session->pageTextContains("An upgrade has already been performed on this site. To perform a new migration, create a clean and empty new install of Drupal $this->destinationSiteVersion. Rollbacks are not yet supported through the user interface.");
   +  }
   ...
   +  protected function assertCredentialForm(WebAssert $session) {
   +    $session->pageTextContains('Provide credentials for the database of the Drupal site you want to upgrade.');
   +    $session->fieldExists('mysql[host]');
   +  }
   

   I'm not convinced this tiny assert methods are a good idea. The abstraction make tests harder to read, an extra thing to know about, an extra thing to maintain. DRY is not always correct - especially in tests. I guess it makes the UI easier to update BUT find/replace does exist. Plus as said below i really don't think we should be passing in $session if we do do this.

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -212,21 +175,101 @@ protected function assertIdConflict(WebAssert $session, array $entity_types) {
   -  protected function assertMigrationResults(array $expected_counts, $version) {
   -    // Have to reset all the statics after migration to ensure entities are
   -    // loadable.
   +  protected function assertUpgrade($session, $version, array $entity_counts) {
   +    $session->pageTextContains(t('Congratulations, you upgraded Drupal!'));
   

   I don't think we should be passing $session into these methods because:

   • Other asserts do not do this.
   • This is a testbase call and call $this->assertSession() just fine.
   • If we do do this (let's not) - this one needs a type hint.

   Yes other asserts added here do pass in $session already but two wrongs...

(facepalm) I was reviewing the patch from #32 and explaining the difference between ?: and ??. The patch in #34 fixes that. I am glad that the testbot caught the difference, too: a sign that our test coverage is pretty good!

I compared the patch in #34 to the one in #26. (Almost true.) It makes the changes requested in #31. As for whether or not the 1- or 2-line helper methods are useful, I will just check that they work. I defer to both @alexpott and @quietone on general testing strategy.

I also did a quick review of the patch in #34 as a whole. Most of it looks familiar. ;)

Looking at the changes to the issue summary, I am reminded that it already mentions some of the points that worried me a bit in #28. I also see that it now mentions "helper classes" instead of "helper functions". If that mistake were in the patch, I would have to send the issue back to NW, but I can fix the issue summary myself .

Back to RTBC.

Committed b4dbfc1 and pushed to 9.1.x. Thanks!