Clean up Migrate's test suite

+1 !!

For me, phenaproxima's statement of the problem is excellent. I agree with the proposed resolution, I just don't know enough to comment on how it will be implemented. But, I will gladly accept tests that run slower if I am able to have consistent results and an easier to understand environment.

MigrateDrupal6Test has been a great test, catching many bugs that actually reflect real life and which aren't caught in setUp() because developers kindly setup everything just how they need it for their code to work where as MigrateDrupal6Test relies on running migrations to do your setup, no cheating!

Running the individual tests are much easier to debug than MigrateDrupal6Test and they catch 80% of the bugs. The proposed solution sounds like making every test as slow and as hard to run as MigrateDrupal6Test

Given we've just moved half of the Migrate API around to accommodate builders and are soon to remove load plugins removing this test would probably be a disaster and I don't believe the proposed solution makes things easier.

I don't see why it's so confusing, maybe we just need to do some renaming. The individual tests are simply being reused for their assertions, we could rename something to reflect that?

Well i'm glad we're not actively working on this now because as you say, there are plenty more important things to stabilise.

All the pain of our integration test suite stems from the fact that the ground can shift from under the feet of the assertions.

I agree this certainly can be problematic when you're trying to test one specific type of failure, maybe that's something we could solve by breaking out the assertions trying to catch specific problems into new tests and keeping our current tests which started off trying to test/assert that the data was imported correctly into D8. Those assertions should be the same whether you used the actual migration path or used setUp() to get things started.

+1 to having individual tests actually execute their dependencies. I'm blocked on #2554003: isComplete() should not rely on RESULT_COMPLETED because the current system is mocking only the minimally-necessary data to pass, so changes that require more of the data context to make sense need to do a lot of work fixing tests. In general, as pointed out above, it's problematic that you can make tests pass by manipulating the fixture data, we're much better off testing against realistic data.

No, I don't think we can't keep the current system. Similar to #14, #2510160: d6 Imagecache -> d8 Image module migration should be included in core is also blocked on being able to provide test coverage.

The problem we have is that MigrateDrupal6Test runs all test and runs all tests cases in the same process. This is the fundamental design of the test. This breaks an assumption in testing though. Each test method must run in its own run wrapped by setUp and tearDown. To go even further, in a kernel or web tests like this it is a 100% different process with different databases. To run in these 2 vastly different context is not fair on people writing tests.

I'll go even further, we have to be able to support the standard method going forward because we can't just test "migrations work!" like is the assumption MigrateDrupal6Test. Qe also have to be able to setup certain invalid states and confirm that the fail correctly. If you change the state in a migration test now it completely breaks all other assertions when MigrateDrupal6Test runs through them.

While I'm not going to argue that this has caught real use cases in the past, that is awesome. The fact is it just isn't sustainable going forward and we're going to have to just build better test coverage of our code and rely on that instead.

Just to be clear, it's not the assertions that are the problem. The problem is the state of the system in which the assertions are being made. We can't allow it to differ based on how the test was run.

This is the whole point of the test, to assert that running all the migrations gives the same outcome as what we got in manually constructed setUp(). The data shouldn't be different as the individual tests should only assert data that came from the dumps.

+1 to having individual tests actually execute their dependencies.

This just sounds like you're turning every test into something as complex as MD6T to me, debugging one migrate test will become a huge PITA, stepping through half a dozen individual migrations :/ This is half of the pain of debugging MD6T, when you have a fail in migration 5, you have to step through 4 migrations to get there, slow and tedious.

While I'm not going to argue that this has caught real use cases in the past, that is awesome. The fact is it just isn't sustainable going forward and we're going to have to just build better test coverage of our code and rely on that instead.

Well it's not just real bugs, it's probably the only test that actually catches anything meaningful right now, including most importantly changes to other parts of core that broke Migrate, very important!

I'll go even further, we have to be able to support the standard method going forward because we can't just test "migrations work!" like is the assumption MigrateDrupal6Test.

This I agree with, I think we could look at some how having our current tests that work within MD6T and then additional tests outside of that for specifically testing other conditions such as testing invalid state etc.

I don't find that to be the case, more often than not, you debug the individual tests and then MD6T is working unless you have a genuine problem.

MD6T is painful to debug when you get a bad issue, if every test had the same debugging process, it would be excruciating.

Can you give me an example? I can't think of many occassions where a single test chokes MD6T where it hasn't either been a) a simple problem in setUp() or b) an actual bug

Yes, I can't write a complete test for #2510160: d6 Imagecache -> d8 Image module migration should be included in core. It requires a database with deprecated values that can't be migrated and so will stop the migration and tell the user why the migration can't complete.

Edit: Clarity, they where deprecated in 6. removed in 7 so don't exist in 8. there is no update path for 7 and no way to translate them or migrate them to 8.

@neclimdul, could make the changes in setUp() as needed or import a new fixture for that one test?

However, I do see the clumsyness in that approach and as I previously mentioned, we could have tests that aren't wrapped up in MD6T.

Just for reference, here are a couple of bugs that were only caught by MigrateDrupal6Test that I could remember. I'm sure there has probably been more.

https://www.drupal.org/node/2414651#comment-10227845
https://www.drupal.org/node/2374087#comment-9342947
https://www.drupal.org/node/2247903#comment-8806547
https://www.drupal.org/node/2346039#comment-9282251 (fixed multiple issues here)

I'm not arguing that running the dependant migrations wouldn't have caught those issues, simply giving everyone a few example issues to test against if we remove this test.

I don't how to say this without coming off wrong but I'm going to do my best. I really do appreciate your input and as I've said respect the way MD6T has slingshotted the migrate development. But I don't think we are going to agree on this because I don't really care what things where caught by this test. If we miss something, and it doesn't get caught by our tests we didn't have sufficient coverage and we write better tests, iterate, iterate, we have a solid test suite. This is actively blocking that process though which is why I keep pushing for its removal.

The current state of things with todays @group opt ins I think makes things ok because its easy enough to opt out of this test. If that where to change I would _strongly_ push for this again. An it is still a terrible trap for test developers though and I don't like it.

I'm open to running the dependant migrations as a possible solution although I foresee that making all migration tests much harder to debug.

It's fine everyone not liking this test but I haven't seen any proposals other than above that offer a solution to catch/test bugs that this test currently covers. It should stay until that point.

1. +++ b/core/modules/book/src/Tests/Migrate/d6/MigrateBookTest.php
   @@ -17,39 +17,16 @@
   -    $this->installEntitySchema('node');
   

   Not needed anymore? Did something change in book migration since this was written or subtle change?

2. +++ b/core/modules/comment/src/Tests/Migrate/d6/MigrateCommentVariableDisplayBase.php
   @@ -15,57 +15,22 @@
   -    entity_create('field_storage_config', array(
   -      'entity_type' => 'node',
   -      'field_name' => 'comment',
   -      'type' => 'comment',
   -      'translatable' => '0',
   -    ))->save();
   -    foreach ($this->types as $type) {
   -      entity_create('node_type', array('type' => $type))->save();
   -      entity_create('field_config', array(
   -        'label' => 'Comments',
   -        'description' => '',
   -        'field_name' => 'comment',
   -        'entity_type' => 'node',
   -        'bundle' => $type,
   -        'required' => 1,
   -      ))->save();
   -    }
   -    $id_mappings = array(
   -      'd6_comment_field_instance' => array(
   -        array(array('page'), array('node', 'comment', 'page')),
   -      ),
   -    );
   -    $this->prepareMigrations($id_mappings);
   -    $this->executeMigration(static::MIGRATION);
   -
   +    $this->installConfig(['comment', 'node']);
   +    $this->executeMigration('d6_node_type');
   +    $this->executeMigration('d6_comment_type');
   +    $this->executeMigration('d6_comment_field');
   +    $this->executeMigration('d6_comment_field_instance');
   

   Gosh darn this just just so much easier to understand.

3. +++ /dev/null
   @@ -1,100 +0,0 @@
   -abstract class MigrateUploadBase extends MigrateDrupal6TestBase {
   

   I assume since this was all setup that the shared code isn't needed anymore.

4. +++ b/core/modules/file/src/Tests/Migrate/d6/MigrateUploadTest.php
   @@ -14,22 +16,44 @@
   +    // Create new file entities.
   +    for ($i = 1; $i <= 3; $i++) {
   +      $file = File::create(array(
   +        'fid' => $i,
   +        'uid' => 1,
   +        'filename' => 'druplicon.txt',
   +        'uri' => "public://druplicon-$i.txt",
   +        'filemime' => 'text/plain',
   +        'created' => 1,
   +        'changed' => 1,
   +        'status' => FILE_STATUS_PERMANENT,
   +      ));
   +      $file->enforceIsNew();
   +      file_put_contents($file->getFileUri(), 'hello world');
   +
   +      // Save it, inserting a new record.
   +      $file->save();
   +      $id_mappings['d6_file'][] = array(array($i), array($i));
   +    }
   

   why do we still need to mock these?

5. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
   @@ -70,20 +70,4 @@ protected function installMigrations($version) {
   -  /**
   -   * Test that the source plugin provides a valid query and a valid count.
   -   */
   -  public function testSourcePlugin() {
   -    if (isset($this->migration)) {
   -      $source = $this->migration->getSourcePlugin();
   -      // Make sure a SqlBase source has a valid query.
   -      if ($source instanceof SqlBase) {
   -        /** @var SqlBase $source */
   -        $this->assertTrue($source->query() instanceof SelectInterface, 'SQL source plugin has valid query');
   -      }
   -      // Validate that any source returns a valid count.
   -      $this->assertTrue(is_numeric($source->count()), 'Source plugin returns valid count');
   -    }
   -  }
   -
   

   while this test was a sneaky sometimes annoying one, is removing it in scope?

6. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
   @@ -14,6 +15,16 @@
    
   +  public static $modules = [
   

   This and probably others should have an inherit doc.

7. +++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeTest.php
   @@ -23,14 +23,12 @@ class MigrateNodeTest extends MigrateNodeTestBase {
   -    // on story and test_planet nodes.
   -    $this->executeMigration('d6_node__test_planet');
   
   @@ -83,10 +76,6 @@ public function testNode() {
   -
   -    $node = Node::load(3);
   -    // Test that format = 0 from source maps to filtered_html.
   -    $this->assertIdentical('filtered_html', $node->body->format);
   

   I admit, these removals aren't clear to me.

8. +++ b/core/modules/user/src/Tests/Migrate/d6/MigrateUserProfileValuesTest.php
   @@ -37,99 +33,12 @@ class MigrateUserProfileValuesTest extends MigrateDrupal6TestBase {
   +    $this->executeMigration('user_profile_field');
   +    $this->executeMigration('user_profile_field_instance');
   +    $this->executeMigration('user_profile_entity_display');
   +    $this->executeMigration('user_profile_entity_form_display');
   +    $this->migrateUsers(FALSE);
   

   I guess this worked but seems reversed.

1. 2. 3. Thanks
4. ok. Maybe we can follow up with a vfs fix but that works for now.
5. eeeegh ok? not sure about it.
6. There is no standard I could see but there is precedence in places like Drupal\Core\Menu\MenuLinkDefault::overrideAllowed
7. Works for me.
8. I just would have guessed the normal user migration would have to go before the profile stuff. Seems to work though so ok.

1. +++ b/core/modules/comment/src/Tests/Migrate/d6/MigrateCommentTest.php
   @@ -19,10 +19,13 @@ class MigrateCommentTest extends MigrateDrupal6TestBase {
   +    // Directly testing that comments' entity_id is populated upon importing is
   

   While moving the comment (which I'm sure I wrote), worth improving it to be clear that the entity_id issue is something that happens to stubs (or at least used to). "Directly testing that a stub comment's..." Don't reroll just for this, of course...

2. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
   @@ -70,20 +70,4 @@ protected function installMigrations($version) {
   -  public function testSourcePlugin() {
   

   I'm going to test locally to see if the issue this was designed to catch (specific source plugins not properly implementing query() and count() - #2499793: Several migrate_drupal migrations fatal error on count()) get caught without it.

Here's a witch-dunking test - it reintroduces one of the buggy conditions from #2499793: Several migrate_drupal migrations fatal error on count(). If this patch fails, that means it is detecting the problem and everything's hunky-dory. If this patch succeeds, this means that without testSourcePlugin() we're no longer testing for the regression, and we need to either bring it back or find another way to generally test for source plugins with bogus query()/count() implementations (or decide we don't need to test that).

Future core committers - DO NOT commit this version of the patch (even if it is green)!

This is now a blocker for #2554003: isComplete() should not rely on RESULT_COMPLETED - the current tests' incomplete fixtures cause failures all over the place with that patch, by properly executing all dependent migrations in this patch most if not all of those failures should go away.

I think you've done a great job here, the tests are certainly much more readable, really good work!

1. +++ b/core/modules/comment/src/Tests/Migrate/d6/MigrateCommentVariableEntityDisplayTest.php
   @@ -15,21 +17,19 @@
   +      $component = EntityViewDisplay::load('node.' . $type . '.default')->getComponent('comment');
   

   I wish core would add a method to EVD to replace entity_get_display()

2. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
   @@ -29,4 +40,91 @@ protected function getDumpDirectory() {
   +  protected function migrateUsers($include_pictures = TRUE) {
   ...
   +  protected function migrateContent($with_revisions = FALSE) {
   

   Maybe $include_revisions?

3. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
   @@ -29,4 +40,91 @@ protected function getDumpDirectory() {
   +    $this->executeMigration('d6_taxonomy_vocabulary');
   +    $this->executeMigration('d6_vocabulary_field');
   +    $this->executeMigration('d6_vocabulary_field_instance');
   +    $this->executeMigration('d6_vocabulary_entity_display');
   +    $this->executeMigration('d6_vocabulary_entity_form_display');
   

   We have a hardcoded dependency list here, I think what would be a better approach would be for executeMigration to take an array and then use loadMultiple() to allow the storage controller to resolve the dependencies. That would make our tests a little more robust.

Also, NW for #33

RTBC +1

With executeMigration(), we could have made migrateExecution() work with both or maybe we should just remove executeMigration() entirely now and only use executeMigrations()?

easy enough.

forgot to mention, conflict was that #2569703: When migrating to the same theme with different region names, block will not migrate properly changed the theme name used in d7/MigrateBlockTest so there was a chunk conflict with some cleanups.

Use executeMigrations in d7/MigrateBlockTest.

Wow, this is a fantastic clean-up! Impressive diffstat as well. :)

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/DummyQueryTrait.php
   @@ -26,4 +26,11 @@ public function query() {
   +  public function count() {
   +    return 1;
   +  }
   

   This stood out as something not like the rest of the changes...

   It turns out this was part of an earlier attempt to move unit test assertions (e.g. number of rows returned by source plugin) out of integration tests.

   It doesn't hurt anything for now, and will be part of a follow-up issue (#2573559: Remove MigrateDrupalTestBase::testSourcePlugin()).

2. +++ /dev/null
   @@ -1,103 +0,0 @@
   -abstract class MigrateFullDrupalTestBase extends MigrateDrupalTestBase {
   

   This is a lot of removed code. However, @phenaproxima pointed out that this doesn't have any assertions in and of itself. And so as a result, we end up doing this work twice. Now, the integration tests will set up their migrations properly with ->executeMigrations()

3. +++ b/core/modules/user/src/Tests/Migrate/MigrateUserProfileEntityFormDisplayTest.php
   @@ -17,81 +17,23 @@
   -    // Create some fields so the data gets stored.
   -    entity_create('field_storage_config', array(
   -      'entity_type' => 'user',
   -      'field_name' => 'profile_color',
   -      'type' => 'text',
   -    ))->save();
   -    entity_create('field_storage_config', array(
   -      'entity_type' => 'user',
   -      'field_name' => 'profile_biography',
   -      'type' => 'text_long',
   -    ))->save();
   ...
   -    foreach ($field_data as $field) {
   -      entity_create('field_config', array(
   -        'label' => $field->title,
   -        'description' => '',
   -        'field_name' => $field->name,
   -        'entity_type' => 'user',
   -        'bundle' => 'user',
   -        'required' => 1,
   -      ))->save();
   -    }
   -
   -    $this->executeMigration('user_profile_entity_form_display');
   +    $this->executeMigrations([
   +      'user_profile_field',
   +      'user_profile_field_instance',
   +      'user_profile_entity_form_display',
   +    ]);
   

   This is really nice to be able to eliminate that much code with just 3 lines of human-legible stuff. :)

No complaints here! Committed and pushed to 8.0.x. Thanks!!