Add helper functions to MigrateDrupal7TestBase

Looks perfect, but please alter the existing Drupal 7 tests to use these methods.

Looking better and better. Few things --

1. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
   @@ -17,10 +18,109 @@
   +      $this->installEntitySchema('file');
   +      $this->executeMigrations([
   +        'user_picture_field',
   +        'user_picture_field_instance',
   +        'user_picture_entity_display',
   +        'user_picture_entity_form_display',
   +      ]);
   

   d7_file also needs to be executed.

2. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
   @@ -17,10 +18,109 @@
   +      // These are optional dependencies of d7_user, but we don't need them if
   +      // we're not migrating user pictures.
   +      Migration::load('user_picture_entity_display')->delete();
   +      Migration::load('user_picture_entity_form_display')->delete();
   

   If pictures are not needed, we don't need user_picture_field or user_picture_field_instance either.

3. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
   @@ -17,10 +18,109 @@
   +      'd7_upload_field',
   +      'd7_upload_field_instance',
   

   These don't exist :)

4. +++ b/core/modules/node/src/Tests/Migrate/d7/MigrateNodeTest.php
   @@ -36,22 +36,13 @@ class MigrateNodeTest extends MigrateDrupal7TestBase {
   +    $this->migrateContent(TRUE);
   

   Is it necessary to migrate the revisions in this test?

5. +++ b/core/modules/user/src/Tests/Migrate/d7/MigrateUserTest.php
   @@ -29,15 +29,7 @@ class MigrateUserTest extends MigrateDrupal7TestBase {
   +    $this->migrateUsers(TRUE);
   

   Likewise, is it necessary to migrate pictures in this test? Unless the test is asserting on the pictures themselves, there's no need to.

Okay, fixed MigrateCommentTest. Fly, baby, fly!

Sigh. No? How 'bout...

+++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
@@ -17,10 +18,121 @@
+  protected function migrateUsers($include_pictures = TRUE) {
...
+  protected function migrateContentTypes() {
...
+  protected function migrateFields() {

I think these helpers are really good for testing, they handle a lot of setUp that can be difficult to get right.

I read the backscroll on IRC, chx mentioned his concerns with the large amount of $modules we're enabling on the base. I do see his point, couldn't we move any module setUp into the helpers methods?

Eg, migrateContent() would install the node module etc. The field modules, we should probably just move those into their relevant tests, surely there can't be many tests depending on the datetime module?

1. +++ b/core/modules/file/config/schema/file.destination.schema.yml
   @@ -5,3 +5,6 @@ migrate.destination.entity:file:
   +    source_base_path:
   +      type: string
   +      label: 'Source base path'
   

   This looks like a non-sequitor?

2. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
   @@ -17,10 +18,121 @@
   +      $destination['source_base_path'] = $this->siteDirectory;
   

   I guess that schema addition above has something to do with this?

@svendecabooter, thx for your analysis. I came to the same conclusion and retired before posting.

Looks like this issue is to create helper functions in MigrateDrupal7TestBase just like in MigrateDrupal6TestBase. And that that work has been competed and reviewed. But this is held up on the concern about the large of $modules being enabled on the base. That doesn't seem to have a simple solutions (#13) and it effects both D6 and D7, which is outside the scope of this issue. So that is being moved to a new issue.

That will also help #2409435: Upgrade path for Book 6.x and 7.x, which is postponed on this issue.

The new issue is #2623280: Minimize enabled modules in D6 and D7 test base

Re mikeryan's comment about source_base_path. That was added in patch #11 by phenaproxima. I've just asked him about it on IRC, he is AFK now but will look later.

I presume that was to fix this error from patch #9,

exception: [Notice] Line 94 of core/modules/file/src/Plugin/migrate/source/d7/File.php:
Undefined index: source_base_pathDrupal\file\Plugin\migrate\source\d7\File->prepareRow(Object)
Drupal\migrate\Plugin\migrate\source\SourcePluginBase->next()
Drupal\migrate\Plugin\migrate\source\SourcePluginBase->rewind()
Drupal\migrate\MigrateExecutable->import()
Drupal\migrate\Tests\MigrateTestBase->executeMigration(Object, 'd7_file')
array_walk(Array, Array)
Drupal\migrate\Tests\MigrateTestBase->executeMigrations(Array)
Drupal\migrate_drupal\Tests\d7\MigrateDrupal7TestBase->migrateUsers(1)
Drupal\user\Tests\Migrate\d7\MigrateUserTest->setUp()
Drupal\simpletest\TestBase->run(Array)
simpletest_script_run_one_test('1764', 'Drupal\user\Tests\Migrate\d7\MigrateUserTest')

Regarding #14 -- @quietone is correct. The source_base_path fix was added to address the exceptions @svendecabooter was encountering in #6.

With these minor nits fixed, RTBC from me, although I can't officially RTBC this since I had a hand in writing it ;-)

1. +++ b/core/modules/comment/src/Tests/Migrate/d7/MigrateCommentTest.php
   @@ -19,36 +19,14 @@
   +    $this->migrateContent(FALSE);
   

   Nit: The FALSE is unnecessary -- the parameter is FALSE by default.

2. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
   @@ -17,10 +18,121 @@
   +    $this->executeMigrations([
   +      'd7_comment_type',
   

   The presence of d7_comment_type here is understandable, but jarring for people who don't know the system. Can a comment be added explaining that?

3. +++ b/core/modules/user/src/Tests/Migrate/d7/MigrateUserTest.php
   @@ -29,15 +29,7 @@ class MigrateUserTest extends MigrateDrupal7TestBase {
   +    $this->migrateUsers(TRUE);
   

   No need for TRUE here -- TRUE is the default for the parameter.

Fixes for all of phenaproxima's nits.

+++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
@@ -17,10 +18,122 @@
+  protected function migrateUsers($include_pictures = TRUE) {
...
+  protected function migrateContentTypes() {
...
+  protected function migrateFields() {
...
+  protected function migrateContent($include_revisions = FALSE) {
...
+  protected function migrateTaxonomy() {

Maybe better to make this methods with trait? like \Drupal\comment\Tests\CommentTestTrait

@andypost, there's no real point in putting those methods in a trait -- the Drupal 6 and Drupal 7 versions are subtly different, and all tests which use them will extend from either MigrateDrupal6TestBase or MigrateDrupal7TestBase already. So putting them in a trait wouldn't add anything -- additionally, the trait would not be able to specify modules to enable for the test (as far as I know, SimpleTest doesn't scan for $modules properties imported from traits).

The patch looks good to me.

Overall, this is a really great patch that should make writing migrate tests cleaner and more reliable.

One thing stood out for me though when I did a code review.

+++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateDrupal7TestBase.php
@@ -17,10 +18,122 @@
+  public static $modules = [
+    'comment',
+    'datetime',
+    'entity_reference',
+    'filter',
+    'image',
+    'link',
+    'node',
+    'taxonomy',
+    'telephone',
+    'text',
+  ];
...
+    $this->installEntitySchema('comment');
+    $this->installEntitySchema('file');
+    $this->installEntitySchema('node');
+    $this->installEntitySchema('taxonomy_term');

I have a lot of concern about all these modules being enabled in every Migrate test. There are two non-trivial problems with this:

• Adding a lot of overhead to the test.
• The test being inappropriately coupled and the test environment being polluted by data and code that is not part of the test.

I see this was also discussed earlier on the issue and that #2623280: Minimize enabled modules in D6 and D7 test base was filed to address it. At a minimum, this hunk of code should have an @todo referencing that issue. However, for me, my initial reaction is that it needs to be blocking rather than a followup, because it does have a negative impact on the whole Migrate test suite.

I see also in #13:

The problem I encounter is when a module gets enabled later on in the process, their migration_templates aren't available.

Maybe the solution to this is to also add functionality to the base class to ensure this gets done? Alternately, we could add support in setUp() (so then the child classes have to do two things, opt in to the correct installed modules in their setup, and then execute the migrations).

If we could solve that in this issue and close #2623280: Minimize enabled modules in D6 and D7 test base as a duplicate that would be excellent. For me it is an important part of the scope of this improvement.

Thanks @svendecabooter and @quietone for working through much of this already!

#13

Only the migration templates for the enabled module should be created,

I agree with that and have used the provider property from #2569805: For Drupal migration, identify the source module to achieve it. There is a new method, findTemplatesByInstalledModule, which scans all templates and returns only those templates where the value of the provider property matches the name of an installed module.

Also, a check is added in installMigrations to prevent the saving of an already saved migration because when that happens, an exception is thrown. There is probably a better way to do this?

I'm curious to see what the testbot says with a combination of 2578263-25.patch and the current patch from #2569805: For Drupal migration, identify the source module.

Rerolling patch against branch 8.1.x-dev.

@Sonal.Sangale, thanks for working on this. It looks like this is a reroll of the patch in #26. That was just a test which combined patch #25 with one from another issue. I think you'll need to start from the patch in #25. It is unfortunate that I didn't run tests on it, that was an oversight.

Re-rolling the patch from #25.

@svendecabooter, Please re-roll your patch..

Re-rolled the patch.

#41 needs reroll and investigate why tests failed (if still applicable).

Your branch is up-to-date with 'origin/8.3.x'.
nothing to commit, working directory clean
$ curl https://www.drupal.org/files/issues/add_helper_functions_to-2578263-41.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 15576  100 15576    0     0  25729      0 --:--:-- --:--:-- --:--:-- 25702
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityDisplayTest.php
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplaySubjectTest.php
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplayTest.php
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentFieldInstanceTest.php
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentFieldTest.php
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentTest.php
Hunk #1 FAILED at 14.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentTest.php.rej
patching file core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentTypeTest.php
patching file core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php
Hunk #1 succeeded at 148 (offset 3 lines).
patching file core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceWidgetSettingsTest.php
Hunk #1 FAILED at 42.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceWidgetSettingsTest.php.rej
patching file core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
patching file core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTypeTest.php
patching file core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTest.php
patching file core/modules/tracker/tests/src/Kernel/Migrate/d7/MigrateTrackerNodeTest.php
Hunk #1 FAILED at 32.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/tracker/tests/src/Kernel/Migrate/d7/MigrateTrackerNodeTest.php.rej
patching file core/modules/tracker/tests/src/Kernel/Migrate/d7/MigrateTrackerUserTest.php
Hunk #1 FAILED at 32.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/tracker/tests/src/Kernel/Migrate/d7/MigrateTrackerUserTest.php.rej
patching file core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
Hunk #1 FAILED at 25.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php.rej

Lets see if we can get this checked out today.

Rerroll of #41

Still needs reroll, methinks.

Here's the updated patch for this issue.

Bring this patch up-to-date.

Fix test failure.

Self-assigning for review.

Needs a reroll.

Rerolled

1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -9,6 +12,8 @@
   +  public static $modules = [];
   +
   

   I don't think we need this if there's not going to be anything in it.

2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +29,124 @@ protected function getFixtureFilePath() {
   +  /**
   +   * Executes all field migrations.
   +   */
   +  protected function migrateFields() {
   +    $this->executeMigration('d7_field');
   +  }
   

   IMHO, unless there's a compelling reason not to, this one method should also migrate the field instances as well. It doesn't seem too likely that we'll want to migrate field storages but not field instances, and if that case arises, we can just execute those migrations manually.

3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +29,124 @@ protected function getFixtureFilePath() {
   +    $this->createType('page');
   +    $this->createType('article');
   +    $this->createType('blog');
   +    $this->createType('book');
   +    $this->createType('forum');
   +    $this->createType('test_content_type');
   

   This gives me pause. Shouldn't we simply migrate the node and comment types for real instead?

4. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +29,124 @@ protected function getFixtureFilePath() {
   +    Vocabulary::create(['vid' => 'test_vocabulary'])->save();
   

   Same here -- would it not be better to simply migrate the taxonomy vocabularies from D7?

5. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +29,124 @@ protected function getFixtureFilePath() {
   +  protected function migrateContentTypes() {
   +    $this->installConfig(['node']);
   +    $this->executeMigration('d7_node_type');
   +  }
   

   This should probably also install schema and config for comments too.

6. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +29,124 @@ protected function getFixtureFilePath() {
   +    $modules = [
   +      'comment',
   +      'datetime',
   +      'image',
   +      'link',
   +      'node',
   +      'taxonomy',
   +      'telephone',
   +      'text',
   +    ];
   +    $this->enableModules($modules);
   

   Nit: No need for $modules to be its own variable.

7. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +29,124 @@ protected function migrateContent() {
   +    $this->installConfig('node');
   

   migrateContent() already calls migrateContentTypes(), which in turn already installs node's config. No need to do it twice.

1. Removed.
2. The only place that does not require the migration of field instances along with fields is MigrateFieldTest (so I have simply used $this->executeMigration('d7_field'); in there.
3. In my mind the intention was to save time, but a couple of non-scientific local tests suggested the difference is about half a second which is likely to be even less for the testbot.
4. See 3.
5. This can't install schema and config for comments because not all tests calling it have the comment module enabled. So I have created migrateCommentTypes() which calls migrateContentTypes() and then executes the d7_comment_type migration.
6. I'm not a big fan of inline multi-line arrays, but that's just a personal preference so happy to make the change.
7. I have done this and a lot of other refactoring of migrateContent() because this method was beginning to go down a completely different line, including enabling modules which is not done in the D6 version and just gets complicated!

In my mind the intention was to save time, but a couple of non-scientific local tests suggested the difference is about half a second which is likely to be even less for the testbot.

Well, it's not performance I'm worried about -- I'm wondering why we are creating node types manually in a functional migration test, and whether that might have repercussions down the line. Setting up pre-migration state directly undermines the reliability of migration tests, and we have been burned by that kind of thing before (and our tests used to be rife with it, but I worked hard in 2015 to correct that). What is the advantage of creating node types directly, rather than migrating them?

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
@@ -12,8 +9,6 @@
@@ -34,43 +29,12 @@ protected function getFixtureFilePath() {

@@ -34,43 +29,12 @@ protected function getFixtureFilePath() {
    */
   protected function migrateFields() {
     $this->executeMigration('d7_field');
-  }
-
-  /**
-   * Executes all field instance migrations.
-   */
-  protected function migrateFieldInstances() {
-    $this->createType('page');
-    $this->createType('article');
-    $this->createType('blog');
-    $this->createType('book');
-    $this->createType('forum');
-    $this->createType('test_content_type');
-    Vocabulary::create(['vid' => 'test_vocabulary'])->save();
-    $this->migrateFields();
+    $this->migrateCommentTypes();
+    $this->executeMigration('d7_taxonomy_vocabulary');
     $this->executeMigration('d7_field_instance');
   }
 
   /**
-   * Creates a node type with a corresponding comment type.
-   *
-   * @param string $id
-   *   The node type ID.
-   */
-  protected function createType($id) {
-    NodeType::create([
-      'type' => $id,
-      'label' => $this->randomString(),
-    ])->save();
-
-    CommentType::create([
-      'id' => 'comment_node_' . $id,
-      'label' => $this->randomString(),
-      'target_entity_type_id' => 'node',
-    ])->save();
-  }
-
-  /**
    * Executes all user migrations.
    *
    * @param bool $include_pictures

Sorry, re-reading what I wrote I didn't make it clear that I totally agree and have removed any creation of nodes etc and replaced it with migrations.

It might help to attach the latest patch ;-)

No new patch, that's all in #64.

My communication/typing skills need work!
s/have removed/had removed

Any Update on this issue ?

Assigning to myself to see what else is left on this.

Needs a re-roll.

Re rolled.

Re-roll based on the patch in #64.

Patch still applies cleanly. What is left here to review?

Yes, the patch at #80 still applies cleanly to 8.6.x

Attaching the same patch here again for the bot to re-review.

Assigning to myself for review.

Started to review this and so far I have read through the issue and had a brief look at the patch, focusing on the more recent request for changes in #63. There was some concern about the performance and loading of lots of modules in the TestBase. That has been resolved by enabling modules in the individual tests and none in the TestBase, which is the right way to go. There is also the change from creating content to actually migrating it which is was should happen in these test. There are no coding standards errors either. All up, this looks really good and much praise to all for getting it this far.

I'm inclined to RTBC except for two things, 1) I want to read through the patch tomorrow (too late now) and 2) I've written a few patches.

OK read through the patch and found a few things;

1. I grepped for 'd7_comment_type' and found it being used in MigrateNodeTest which is not modified by this patch. Shouldn't MigrateNodeTest use the new helper functions? It was in the patch way back in #25 but got lost after some rerolling.
2. grep also turned up usages of d7_node_type and d7_taxonomy_term in d7 Migrate tests. Is there a reason for this? If so, does this need documentation to explain when to use the helpers and when not to use them? I didn't look further than the grep output so maybe it is obvious?
3. I'd like to see documentation on the helper functions stating what modules need to be installed. It is a nice to have and should not hold this up. If someone thinks it is worth doing, a follow up is fine.
4. Not something for this patch but it would be good to also remove the module list from MigrateTestBase and update all the d6 and d7 test accordingly.

I have addressed points 1 & 2 from #86 (there is one instance of d7_taxonomy_term remaining in *Test.php because that uses CreateTestContentEntitiesTrait and a whole different approach).

Then I ran out of time...

Points 3 & 4 from #86 still need to be addressed, but here's my work in progress.

Having re-read #86 (when not rushing), I see that points 3 & 4 discuss follow-ups so I will set this to Needs Review.

@Jo Fitzgerald, awesome.

I found several occurrences where executeMigrations can be executeMigration.

1. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplaySubjectTest.php
   @@ -23,11 +23,8 @@ class MigrateCommentEntityFormDisplaySubjectTest extends MigrateDrupal7TestBase
   +    $this->executeMigrations(['d7_comment_entity_form_display_subject']);
   

   +++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentFieldTest.php
   @@ -23,11 +23,8 @@ class MigrateCommentFieldTest extends MigrateDrupal7TestBase {
   +    $this->executeMigrations(['d7_comment_field']);
   

   +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +24,80 @@ protected function getFixtureFilePath() {
   +    $this->executeMigrations(['d7_node']);
   ...
   +      $this->executeMigrations(['d7_node_revision']);
   ...
   +    $this->executeMigrations(['d7_taxonomy_term']);
   

   Can be executeMigration.

Corrected the 5 instances of executeMigrations() that could be replaced with executeMigration(). Also found one instance of executeMigration() that should be executeMigrations().

Aha, so there is a difference between executeMigrations(['migration_id']) and executeMigration('migration_id'). The first runs createInstances() which will return migration derivatives, e.g. d7_taxonomy_term:tags.

I have made the necessary corrections and added a comment.

Re-rolled.

@Jo Fitzgerald, oh yes, I too forgot that one of those will create the derivatives. The docs on executeMigrations should be improved to make the obvious.
Although I haven't posted I have been reviewing that changes made due to #89. Everything looks good to go. This is really amazing to sort this out.

The remaining items I found are these, and they are from me!

1. I'd like to see documentation on the helper functions stating what modules need to be installed. It is a nice to have and should not hold this up. If someone thinks it is worth doing, a follow up is fine.
2. Not something for this patch but it would be good to also remove the module list from MigrateTestBase and update all the d6 and d7 test accordingly.Made an issue #2974445: [Meta] Refactor Migrate Drupal UI Functional tests
3. A follow up to improve/correct the documentation on executeMigrations to explain that it will create the derivations.

Do the others need a follow up?

Let's add two follow-ups 95.1 and 95.3 and this could be pretty close to ready.

Created follow-ups in response to:
95.1. #2978287: Add documentation to helper functions in MigrateDrupal7TestBase
95.3. #2978288: Improve the documentation on executeMigrations

I think this is ready for RTBC...

@jo Fitzgerald, thanks for making the followups.

Found one more thing,
The test reports coding standard errors. There are unused use statements to remove in MigrateNodeTaxonomtyTest.

Corrected coding standards errors.

I can't believe I missed them! I hang my head in shame.

Thank you!

Notice this wee thing this time.

1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +24,85 @@ protected function getFixtureFilePath() {
   +    $this->installEntitySchema('file');
   

I think this can go inside the following if block.

2. Then I grepped for all the migrations in the new helper test functions and only found one thing, where it looks like MigrateUsers() can be used in 3 more tests.

   +++ b/core/modules/tracker/tests/src/Kernel/Migrate/d7/MigrateTrackerNodeTest.php
   @@ -33,10 +33,10 @@ protected function setUp() {
          'd7_user_role',
          'd7_user',
   
   +++ b/core/modules/tracker/tests/src/Kernel/Migrate/d7/MigrateTrackerUserTest.php
   @@ -33,10 +33,10 @@ protected function setUp() {
          'd7_user_role',
          'd7_user',
   

Can use MigrateUsers() ? Also, I think MigrateShortcutSetUsersTest.php can as well.

Thats it, almost done!

Addressed the @quietone's comments in #100 and, following a quick review, removed some redundant lines from:

• FollowUpMigrationsTest
• MigrateFieldFormatterSettingsTest
• MigrateFieldInstanceTest
• MigrateFieldInstanceWidgetSettingsTest
• MigrateNodeTaxonomyTest
• MigrateNodeTest
• MigrateUserTest

imo, it would be nice to have a follow-up that handled the $modules variable because the contents are related to the helper functions, so I've created #2978862: Add set-up helper functions to MigrateDrupal7TestBase.

Two small things, then I think we're ready.

1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +24,85 @@ protected function getFixtureFilePath() {
   +   * @param bool $include_pictures
   +   *   If TRUE, migrates user pictures.
   

   Needs (optional) before the description, and let's add "defaults to TRUE".

2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -24,4 +24,85 @@ protected function getFixtureFilePath() {
   +   * @param bool $include_revisions
   +   *   If TRUE, migrates node revisions.
   

   Needs (optional), and let's add "defaults to FALSE".

Addressed @phenaproxima comments in #103, also added an interdiff.

Thanks, @yogeshmpawar! RTBC once all backends pass tests.

I think this is some random Nightwatch BS. Back to RTBC.

A reroll.

Added tests for PostgreSQl and SQLite

Thanks for the reroll! Restoring RTBC.

+++ /dev/null
@@ -1,38 +0,0 @@
-<?php
-
-namespace Drupal\Tests\migrate\Kernel;
-
-use Drupal\comment\Entity\CommentType;
-use Drupal\node\Entity\NodeType;
-
-/**
- * Provides methods for testing node and comment combinations.
- */
-trait NodeCommentCombinationTrait {

Let's deprecate this and tell people to use the new methods and only remove in Drupal 9. That way we won't break contrib or custom tests that use this.

Patch from #112 no longer applies, re-rolled.

Deprecated NodeCommentCombinationTrait.

Unless I'm reading that interdiff wrong, it looks like we also removed the code in the trait? I don't think that is what was intended.

You're absolutely right, @heddn. I was kinda fumbling in the dark with that one. How about this.

That seems more right. Let's wait for tests to pass.

Feedback from #115 addressed. Back to RTBC.

Patch looks good to me now, but needs a re-roll.

Rerolled patch.

Re-rolled patch from #119 (included interdiff against #123, for reference).

I looked at this last night and even started to make a patch when I noticed the time and went to bed. Jo Fitzgerald has implemented what I started for taxonomy and he has done better and fixed all the tests. I like the removal of these lines as well. Don't think I would have spotted that there were not needed last night.

+++ b/core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageContentSettingsTest.php
@@ -24,7 +24,6 @@
-    $this->installConfig(['node']);

+++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTest.php
@@ -42,8 +42,16 @@
-    $this->installEntitySchema('node');

Back to RTBC

Committed 4b9af62 and pushed to 8.7.x. Thanks!