Problem/Motivation

Since #2534158: MigrateFullDrupalTestBase must use dynamic test discovery, MigrateFullDrupalTestBase has used the test_discovery service to discover the migration tests to be executed. This worked well for a while, but it has recently begun causing testbot failures due to using an enormous amount of memory. I and other Migrate developers have consistently found that the discovery code alone in MigrateFullDrupalTestBase causes memory usage to balloon from about 85 MB to 250 MB! Since this causes testbot failures, other patches cannot be reviewed or committed, making this a "nuclear-strength" Migrate critical bug.

Proposed Resolution

Rather than checking the tests' ancestry in MigrateFullDrupalTestBase::getTestClassesList(), which causes the test classes to be parsed and loaded into memory (thus chewing it all up), tests to be run by subclasses of MigrateFullDrupalTestBase should belong to a specific group. Indeed, this approach substantially reduces memory usage to no more than (for me, anyway) 90 MB before the actual tests are run.

Remaining Tasks

  • Change MigrateFullDrupalTestBase to use group-based discovery.
  • Move all Drupal 6 migration tests into the migrate_drupal_6 test group.
  • Review the patch
  • Commit the patch
  • Breathe a HUGE sigh of relief and repair to the nearest dive bar
CommentFileSizeAuthor
migrate-memory_fix.patch46.33 KBphenaproxima

Comments

phenaproxima created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented 
  1. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php
@@ -17,11 +17,7 @@
-  protected static $blacklist = array(
-    'Drupal\migrate_drupal\Tests\dependencies\MigrateDependenciesTest',
-  );

    Thats an amusing side effect. 

  2. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php
@@ -52,6 +48,7 @@ class MigrateDrupal6Test extends MigrateFullDrupalTestBase {
+    'path',

    I don't think this is a problem but just for reviewer sanity why is this getting added?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Regarding #2: Because without it MigrateUrlAliasTest will not run. The d6_url_alias migration template is included with the Path module, and the effect of the Great Migration Migration has been that any migrations to be tested need to have their modules enabled.

neclimdul’s picture

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

Assuming PIFR will figure out its weirdness and pass this eventually.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

+1

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This is gumming up all the migrate works, makes the group names more accurate, reduces testbot memory consumption by an insane amount (~66%?), and also gets rid of that goofy blacklist stuff. Win++.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 8aee36f on 8.0.x 
    Issue #2557253 by phenaproxima, neclimdul: MigrateFullDrupalTestBase...

Status: Fixed » Closed (fixed)

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