Convert migrate_drupal tests to KernelTestBase

i believe this will still fail, but it's re-rolled for psr-4 conversion, and replaces DrupalUnitTestBase with KernelTestBase.

Somewhere since the original patch MigrateDrupalTestBase already acquired a setUp(), merged this one into it (although I'm not clear why it's necessary to explicitly install migrate_drupal's config, shouldn't it be installed automatically?).

The migrate_drupal tests all fail locally for me with or without this patch ("Table system already exists" - maybe #2254157: Refactor Dump classes in migrate_drupal will fix this?), but giving the testbot a shot...

Reading the KernelTestBase docs, I see "Unlike WebTestBase::setUp(), the specified modules are loaded only, but not automatically installed. Modules need to be installed manually, if needed.". I though KernelTestBase::enableModules() would address this, but found only sadness. What works for the aggregator tests is adding $this->installEntitySchema('aggregator_feed') - some tedious work ahead to run through the failing tests and explicitly install the bits they need.

Checkpoint, I'll give the testbot a shot at it but there's a lot more to do here. MigrateBookTest is as far as I got and I didn't quit work out the magic invocation for that one. Marking Novice - I think given the ones that are done, most of the remaining tests should be doable with just a bit of trial-and-error.

I had a feeling that finishing this patch will fix the last remaining migrate test fail for SQLite, and it seems that it is indeed the case.

This was not a novice issue at all so I'm removing the tag.

Edit: removed the long test run result from the comment, it wasn't really helping with anything :)

Fixed the unit test.

1. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
   @@ -111,10 +111,10 @@ public function testEnsureTablesNotExist() {
    
   -    $schema->expects($this->at(2))
   +    $schema->expects($this->at(3))
   

   should we also define the new call at 2 in the unit test?

2. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
   @@ -15,19 +15,22 @@
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = array('system', 'user', 'field', 'migrate_drupal', 'options');
   

   I guess some modules would make more sense in the parent class, but doesn't make a big difference...

3. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php
   @@ -158,6 +164,37 @@ protected function setUp() {
   +    foreach (static::$modules as $module) {
   +      $function = $module . '_schema';
   +      module_load_install($module);
   +      if (function_exists($function)) {
   +        $schema = $function();
   +        $this->installSchema($module, array_keys($schema));
   +      }
   +    }
   

   That's.. creative ;)

   Maybe that's why it requires the table create change? Or it is sqlite, as you said.

4. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php
   @@ -158,6 +164,37 @@ protected function setUp() {
   +    $this->installConfig(['block_content', 'comment', 'file', 'node', 'simpletest']);
   

   simpletest? ;) we are migrating the config, it shouldn't matter if it already exists or not, or does it? I guess something from simpletest.module runs that needs its config?

5. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php
   @@ -158,6 +164,37 @@ protected function setUp() {
   +    // Create a new user. This is needed by
   +    // \Drupal\migrate_drupal\Tests\d6\MigrateNodeTestBase
   

   needs a . at the end I think ;)

   I'd suggest to explicitly use uid = 1, and document that it needs user *1*, not just any user. You can make it save with enforceIsNew().

6. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateNodeTestBase.php
   @@ -9,19 +9,32 @@
   +    // Create a new user.
   +    User::create([
   +      'name' => $this->randomMachineName(),
   +      'status' => 1,
   +    ])->save();
   

   duplicated here? shouldn't be necessary then, or actually, you create two users?

Thanks for reviewing :)

1: Why not, fixed.
2: I wanted to keep the module list as small as possible in the base class so it is more visible in the child classes what the dependencies are.
3: Thanks :P Nope, that's not the reason for the message table creation change because that's not provided by a hook_schema() imeplementation.
3, 4 & 6: The thing with MigrateDrupal6Test::setUp() is that it needs to duplicate the work being done in the setUp() methods of *all* the migrate_drupal test classes, and one of them installs the simpletest config :)
5: Fixed.

Improved the title a bit, there's no such thing as DUTB anymore.

And with this small change in MigrateUserProfileFieldTest we have MigrateDrupal6Test fully passing on DrupalCI testbots. I blame it (again) on race conditions caused by the weird way MigrateDrupal6Test chooses to run all the migrate_drupal test classes.

Edit: this is the actual fail if anyone is curious:

<testcase classname="Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test" name="testUserProfileFields()"><failure type="failure" message="Other">Value array (
  &amp;#039;Pill spammers&amp;#039; =&amp;gt; &amp;#039;Pill spammers&amp;#039;,
  &amp;#039;Fitness spammers&amp;#039; =&amp;gt; &amp;#039;Fitness spammers&amp;#039;,
  &amp;#039;Back\\slash&amp;#039; =&amp;gt; &amp;#039;Back\\slash&amp;#039;,
  &amp;#039;Forward/slash&amp;#039; =&amp;gt; &amp;#039;Forward/slash&amp;#039;,
  &amp;#039;Dot.in.the.middle&amp;#039; =&amp;gt; &amp;#039;Dot.in.the.middle&amp;#039;,
  &amp;#039;Anonymous donor&amp;#039; =&amp;gt; &amp;#039;Anonymous donor&amp;#039;,
  &amp;#039;Faithful servant&amp;#039; =&amp;gt; &amp;#039;Faithful servant&amp;#039;,
) is identical to value array (
  &amp;#039;Pill spammers&amp;#039; =&amp;gt; &amp;#039;Pill spammers&amp;#039;,
  &amp;#039;Fitness spammers&amp;#039; =&amp;gt; &amp;#039;Fitness spammers&amp;#039;,
  &amp;#039;Back\\slash&amp;#039; =&amp;gt; &amp;#039;Back\\slash&amp;#039;,
  &amp;#039;Forward/slash&amp;#039; =&amp;gt; &amp;#039;Forward/slash&amp;#039;,
  &amp;#039;Dot.in.the.middle&amp;#039; =&amp;gt; &amp;#039;Dot.in.the.middle&amp;#039;,
  &amp;#039;Faithful servant&amp;#039; =&amp;gt; &amp;#039;Faithful servant&amp;#039;,
  &amp;#039;Anonymous donor&amp;#039; =&amp;gt; &amp;#039;Anonymous donor&amp;#039;,
).</failure></testcase>

Added an issue summary.

@amateescu, can you describe why moving it helps to fix the sqlite problem with it?

The code looks really great!

1. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
   @@ -50,16 +50,17 @@
   +      $prefix = is_array($value['prefix']) ? $value['prefix']['default'] : $value['prefix'];
   

   Interesting, indeed with support both.

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
   @@ -112,9 +112,13 @@ public function testEnsureTablesNotExist() {
   +      ->will($this->returnValue(FALSE));
   

   Feel free to use ->willReturnValue() the next time.

3. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
   @@ -112,9 +112,13 @@ public function testEnsureTablesNotExist() {
   -    $schema->expects($this->exactly(3))
   +    $schema->expects($this->exactly(4))
   

   If you change things anyway, can you change this to $this->any() it is just the way better idea here.

can you describe why moving it helps to fix the sqlite problem with it?

Do you mean the change from assertIdentical() to assertEqual()? I can not say I understand 100% what's going on, but as I said earlier, it has to be some kind of race condition triggered by the unusual way of running test methods in MigrateDrupal6Test. You can see the actual failure in #22, it's the last two entries of a $field_storage['settings']['allowed_values'] entry that are in the wrong order.

1. Yep, I had no idea :)
2. I cannot find a willReturnValue() method anywhere in PHPUnit..
3. Why not, fixed.

I think the method is named willReturn() :)

Nice, this means the migrate_drupal_migrate test group (87 tests!) runs in 3m instead of 12m, so 4x faster. That's with concurrency 1, not going to be a huge difference on testbot with a concurrency of 14 of course, but still, very nice if you're running them locally.

@dawehner mentioned in IRC that it would make sense to document the changes that were done to fix sqlite but looking through the patch, I'm actually not sure where we could improve documentation.

I think this is ready.

(Kind of crossposted with above)

willReturn()

Ups, yeah!

+1 for the RTBC

This is a lovely change. Thank you. Committed ea32974 and pushed to 8.0.x. Thanks!