This partially blocks #2549013: Remove load plugins.

If we simply remove load plugins and their infrastructure outright, MigrateDrupal6Test breaks because it tries to test dynamic migrations (i.e., migrations which use load plugins, like d6_cck_field_values, d6_term_node, and a couple of others).

What it needs to be doing instead is running the appropriate builders when the migrations are installed to begin with. However, just running the builders in installMigrations() isn't good enough, because builders usually need to read from arbitrary database tables in order to do their thing. If the required tables haven't been loaded up first, things blow up in an inferno of PDO exceptions.

It's also not good enough to simply change the loadDumps() calls to include the tables which the builders will need. Why? Because installMigrations() ham-fistedly installs all available migrations, not just the one(s) to be tested. So all builders would end up running (and failing due to missing tables), even those which have nothing to do with the migrations under test.

There are, then, two options:

  1. Load all the dump files beforehand so that the builders have all the data they need, up front. This is relatively easy and allows us to remove over 300 lines of code, so that's why I did that in the patch.
  2. setUp() can manually install the migrations to be tested, load in the dump files required by the builders beforehand. This would take longer, add more code, add another step to the tests' setUp() methods, and be prone to breaking since it's yet another thing to remember.
CommentFileSizeAuthor
#7 interdiff-2550823-0-7.txt1.21 KBphenaproxima
#7 2550823-7.patch57.88 KBphenaproxima
migrate-load_all_tables.patch56.72 KBphenaproxima

Comments

phenaproxima created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented

I have a feeling this is going to be a problem. We currently have a "run all migrations together" test which does a similar sort of thing but it is blocking me from making a dump that has a failure in it. This is going to have the same problem, you can only have one d6 install you test, not multiple states.

I would rather have an approach similar to the update system(or just use the same system) where you have a full site in a single dump. We could have on standard fixture that matches this, then specific tests could use specialized fixtures.

This of course would break the "run all migrations" test because any test could use a different fixture so that's a problem but that's what I would like to see.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes

Rewrote the issue summary to include the rationale for this patch, at @neclimdul's request.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, migrate-load_all_tables.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2550823-7.patch57.88 KB
new interdiff-2550823-0-7.txt1.21 KB

Missed a couple of spots.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

This issue looks fine to me, a bit slower but much less code to manage in tests. I was pleasantly surprised that it was so easy to get green, eg no funky dependencies between dumps and tests.

From speaking with phenaproxima in IRC it sounds like neclimdul (who expressed some concerns) is on board with this so RTBC.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs review

Changing back to NR so it doesn't get committed yet; I'd like to get @neclimdul's sign-off on this first.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Reviewed & tested by the community

It looks fine to me. I'd like to see us standardize on the same approach as update module but we don't need to do that here.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
+++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
@@ -30,7 +30,8 @@

@@ -30,7 +30,8 @@
    */
   protected function setUp() {
     parent::setUp();
-    $this->loadDumps(['System.php']);
+    $tables = file_scan_directory($this->getDumpDirectory(), '/.php$/', array('recurse' => FALSE));
+    $this->loadDumps(array_keys($tables));

This seems like the key part of the patch, and it also seems like it's going to make things much more robust in the future. +100.

If it does slow things down an awful lot, we can always roll this back and maybe do a base vs. full distinction like update path tests do, but let's try this for now.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed d2404be on 8.0.x 
    Issue #2550823 by phenaproxima, neclimdul: Migrate Drupal integration...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.