Write tests

Sounds like a migrate critical?

There are two tests that needs to be written for this:

1) WebTest to check all the states of the upgrade form
2) UnitTest for testing the migration trait.

Patch for the WebTest is attached. Its WIP

Marking NR since there is a patch.

Another patch, still WIP

So the d6_user is migrating a user that replaces the user the test creates and that user does not have permissions to run the batch :)

If we don't create the user and just log on as the test user everything works.

Fixing test

With patch :( d.o is behaving very weird for me.

Fixing D7 test and postgres... can't get sqlite to work yet.

In order to get sqlite to pass we need to fix #2675000: SQLBase::mapjoinable does not support SQLite part 2

Patch attached cleans everything up a bit.

Bit of a head scratcher... something is fragile in Postgres prefixing... anyhow this fixes it so with #2675000: SQLBase::mapjoinable does not support SQLite part 2 we have green on all dbs!

1. +++ b/src/Form/MigrateUpgradeForm.php
   @@ -734,7 +734,17 @@ class MigrateUpgradeForm extends FormBase implements ConfirmFormInterface {
   +    // This does not work unfortunately because this part of the form is not
   +    // re-built on submission because it is a multi-step form.
   

   This comment sounds like it's saying that the code in the patch does not work. If it does work, can we rewrite the comment to describe the current code rather than what doesn't work?

2. +++ b/src/Form/MigrateUpgradeForm.php
   @@ -759,6 +769,11 @@ class MigrateUpgradeForm extends FormBase implements ConfirmFormInterface {
   +      // The #limit_validation_errors in the submit does not work so it is
   +      // not possible to require the database and username for mysql and psql.
   +      // This is because this is a multi-step form.
   +      $form['database']['settings'][$key]['database']['#required'] = FALSE;
   +      $form['database']['settings'][$key]['username']['#required'] = FALSE;
   
   @@ -804,10 +819,10 @@ class MigrateUpgradeForm extends FormBase implements ConfirmFormInterface {
   -      '#limit_validation_errors' => [
   -        ['driver'],
   -        [$default_driver],
   -      ],
   +//      '#limit_validation_errors' => [
   +//        ['driver'],
   +//        [$default_driver],
   +//      ],
   

   Hm, where are these fields set to required in the first place? Wouldn't it be better to just change them to not required wherever that is, rather than overriding it? It wouldn't be coming from the parent form in this case.

   I think it would be better to have an @todo and followup issue to figure out how to make #limit_validation_errors work rather than commenting out the code. That would also help explain the disabled requirement on the fields.

1. +++ b/src/Tests/MigrateUpgradeTestBase.php
   @@ -0,0 +1,160 @@
   +
   +
   

   Nit: Extra blank line.

2. +++ b/src/Tests/MigrateUpgradeTestBase.php
   @@ -0,0 +1,160 @@
   + * Base class for Drupal migration tests.
   

   This should be something like "Provides a base class for testing migration upgrades in the UI."

3. +++ b/src/Tests/MigrateUpgradeTestBase.php
   @@ -0,0 +1,160 @@
   +  /**
   +   * User with admin rights.
   +   */
   +  protected $privilegedUser;
   ...
   +    // Create and log in our privileged user.
   +    // Migrating replaces this user and they don't have permissions :)
   +//    $this->privilegedUser = $this->drupalCreateUser([
   +//      'access content',
   +//      'administer site configuration',
   +//      'administer software updates',
   +//    ]);
   

   I guess this should also be removed?

4. +++ b/src/Tests/MigrateUpgradeTestBase.php
   @@ -0,0 +1,160 @@
   +  private function createMigrationConnection() {
   

   Are we sure we want this to be private?

5. +++ b/src/Tests/MigrateUpgradeTestBase.php
   @@ -0,0 +1,160 @@
   +      // Simpletest uses 7 character prefixes at most so this can't cause
   +      // collisions.
   +      $connection_info['prefix'] = $prefix . '0';
   

   I don't understand why this comment means this line, or for that matter why 7-character prefixes at most means it can't cause collisions.

6. +++ b/src/Tests/d6/MigrateUpgrade6Test.php
   @@ -0,0 +1,32 @@
   +/**
   + * @group migrate_upgrade
   ...
   +class MigrateUpgrade6Test extends MigrateUpgradeTestBase {
   
   +++ b/src/Tests/d7/MigrateUpgrade7Test.php
   @@ -0,0 +1,32 @@
   +/**
   + * @group migrate_upgrade
   + */
   +class MigrateUpgrade7Test extends MigrateUpgradeTestBase {
   

   These are missing the one-line summary. Reference: https://www.drupal.org/node/1354#drupal

   I see we are also using the pattern here of having the test method on the base class so that it is executed for all child classes. It's worth adding a note to the class docblocks indicating that the test executes test methods from the base class.

I requeued the tests now that #2675000: SQLBase::mapjoinable does not support SQLite part 2 is in 8.1.x.

Huh, it looks like SQLite is still failing. :(

The sqlite change didn't get pushed... it has been now - requeued the tests

Addressed all the points in #16 and #17 with fixes to comments and code. Re #16

Wouldn't it be better to just change them to not required wherever that is, rather than overriding it?

Nope. The database settings form is from core - so that is not possible.

We still have a problem... adding tests with

  /**
   * Exempt from strict schema checking.
   *
   * @see \Drupal\Core\Config\Testing\ConfigSchemaChecker
   *
   * @var bool
   */
  protected $strictConfigSchema = FALSE;

is very far from ideal.

In order to fix it we would have to add the following keys to core migration schema...

diff --git a/core/modules/file/config/schema/file.destination.schema.yml b/core/modules/file/config/schema/file.destination.schema.yml
index c6b197f..452ff69 100644
--- a/core/modules/file/config/schema/file.destination.schema.yml
+++ b/core/modules/file/config/schema/file.destination.schema.yml
@@ -5,6 +5,9 @@ migrate.destination.entity:file:
     source_path_property:
       type: string
       label: 'Source path'
+    source_base_path:
+      type: string
+      label: 'Source base path'
     urlencode:
       type: boolean
       label: 'Whether to urlencode incoming file paths'
diff --git a/core/modules/migrate/config/schema/migrate.data_types.schema.yml b/core/modules/migrate/config/schema/migrate.data_types.schema.yml
index 27c8006..76af2cc 100644
--- a/core/modules/migrate/config/schema/migrate.data_types.schema.yml
+++ b/core/modules/migrate/config/schema/migrate.data_types.schema.yml
@@ -25,6 +25,9 @@ migrate_source:
     constants:
       type: ignore
       label: 'Constants'
+    database_state_key:
+      type: string
+      label: 'Key in state where the source database settings are stored'
 
 migrate_process:
   type: migrate_plugin

However at least for the database_state_key I'm not convinced that this is correct path to go down.

I've created #2678564: A missing schema to support migrate UI - source_base_path and database_state_key for the file schema.

So actually both are missing schema - so #2678564: A missing schema to support migrate UI - source_base_path and database_state_key will fix both.

I can't think of any decent way of really really testing the migrations - but at the very least we can count all of the entities. It is better than nothing.

#2678564: A missing schema to support migrate UI - source_base_path and database_state_key is in now.

+++ b/src/Tests/MigrateUpgradeTestBase.php
@@ -0,0 +1,173 @@
+  /**
+   * Exempt from strict schema checking.
+   *
+   * @see \Drupal\Core\Config\Testing\ConfigSchemaChecker
+   *
+   * @var bool
+   */
+  protected $strictConfigSchema = FALSE;

So do we remove this now?

Other than that outstanding thing, I think this patch provides sufficient test coverage for the initial alpha. The counts are definitely an improvement.

Let's remove the turning off strict schema checking.

We also need tests for the rollback process. I have written some code for the same, but the tests are failing. Looks like the rollback is not clean.

@abhishek-anand I'd be tempted to but rollback testing into a followup.

Yeah, I think let's put the rollback tests in a followup issue.

I added @abhishek-anand's start at rollback tests in #2678994: Write tests for rollback. Reuploading the earlier patch which is RTBC if postgres doesn't fail again.

The test runs have actually completed on DrupalCI, just not updated on d.o. The postgres fail is a known/recurring DrupalCI issue, so I committed and pushed this to 8.x-1.x. We will have another chance to test it with the core patch, also.