Cleanup migrate drupal functional tests

This patch fixes two things.

1. The test for missing modules is done more than once, it should only be in the SourceProviderTest.
2. The SourceProviderTest should be using the new helper method, getCredentials.

NR for testing

A few more

1. MigrateUpgradeExecuteTestBase
   • Update documentation
   • Remove remnants of running three upgrades since it now doing two. The removed one is now in CredentialFormTest
2. MigrateUpgradeExecuteTestBase and NoMultilingualReviewPageTestBase.php
   • Remove tests of IdConflictForm. The base class should not assume that the d6 and d7 database will cause the same id conflicts. That form is tested in d6 and d7 IdConflictTest.

I plan on doing another review of this in a day or two.

Rerolled because #3145005: [November 9, 2020] Remove uses of t() in drupalPostForm() calls was committed.

Removed $connection_options in SourceProviderTest since it is no longer used, it was moved to the method \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase::getCredentials in #3143719: Add a getCredentials helper method to migrate_drupal_ui functional tests.

The only thing that I see that could be still be done in the test refactoring is to make an IdConflictTestBase. While that would be nice and be consistent with the other Functional tests and it would make maintenance a little easier I am not able to convince myself that it is worth the effort. The additional of the helper methods and pulling duplicate code into two new tests have (see #2974445: [Meta] Refactor Migrate Drupal UI Functional tests) have significantly improved the readability and the maintainability of the functional tests. If, for some reason, we decide later that the IdConflict tests need more work we can revisit making a base class then.

@adityasingh, thanks for the reroll. Can you provide and interdiff as well. If needed there are instructions for creating an interdiff in the handbook.

@adityasingh, I agree that the reroll section on that says that it is not a requirement but that it is nice to have. However, the interdiff or diff is the only thing that will provide proof that another change has not been accidentally introduced in the rerolled patch. I have found that when I have neglected to make a diff a reviewer will request one before they do a review. And now that I do more reviews I understand the point, so that now I almost always make a diff (unless the patch is very small in size) to make life easier for the reviewer and to move the issue along.

That is why I am asking for an interdiff here.

This needs and diff for the latest patch. There are instructions for creating an interdiff in the handbook.

Suitable for a noivce.

@adityasingh, that for coming back and adding the diff. The confirms the reroll is fine and this is ready for review.

@benjifisher, thanks. thorough as usual!

This patch address all the points in #22. It is late in the day and I haven't done a proper self review but let's make sure the tests pass.

Just updating the IS.

#21.3 testMigrateUpgradeExecute is split into two methods, MigrateUpgradeExecuteTestBase::doUpgradeAndIncremental and MigrateUpgradeTestBase::submitCredentialForm. Note the latter is moved to MigrateUpgradeTestBase and so it can then be used in MultilingualReviewPageTestBase and NoMultilingualReviewPageTestBase.

This is ready for another review.

Sure. Here are the changes.

26.1. Fixed.
26.2. The file and rdf modules do not need to be in the list at all. These tests use the standard install profile which includes file and rdf. So, I have removed those and alphabetized the list if the two IdConflict tests and Upgrade tests as well as correcting the doc block for $modules.

I never did the match before but this patch is +68, -219 for net -151.

@ayushmishra206, can you add the diff?

@ayushmishra206, thanks. But that diff looks wrong to me. I downloaded the last two patches and made a diff with diff 3175953-27.patch 3175953-31_0.patch > diff-27-31.txt. That looks better and it helps to identify the error, which is here

311,316c313,318
< -    $this->drupalGet('/upgrade');
< -    $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->submitForm([], 'Continue');
< -    $session->pageTextContains('Provide credentials for the database of the Drupal site you want to upgrade.');
< -    $session->fieldExists('mysql[host]');
---
>      $this->drupalGet('/upgrade');
>      $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.");
> @@ -89,8 +78,8 @@ public function testMigrateUpgradeExecute() {
>      $this->submitForm([], 'Continue');
>      $session->pageTextContains('Provide credentials for the database of the Drupal site you want to upgrade.');
>      $session->fieldExists('mysql[host]');
324c326

The patch in #27 removed lines which are restored in #31. Once they are removed the test should pass.

Cheers.

This was brought up at the migrate meeting noting that it has stalled. To get back on track I am rerolling from #27.

This commit was lovely to see this morning. Thanks catch!