The tests of migrate_drupal repeat a lot of code. Tons of calls to getDumpDirectory() and the three lines required to execute a migration. There's no reason these should not be utility methods of MigrateTestBase. The attached patch does exactly that, and cleans up the tests enormously -- 312 insertions, 919 deletions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, migrate_drupal-clean_tests.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
108.08 KB
9.1 KB

Missed a few spots. Danke, testbot.

Status: Needs review » Needs work

The last submitted patch, 2: 2514168-2.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
108.08 KB
756 bytes

D'oh. Let's play again!

Status: Needs review » Needs work

The last submitted patch, 4: 2514168-4.patch, failed testing.

mikeryan’s picture

Ooh, I've seen that same error in trying to figure out my test problems in #2513028: Migration dependency ordering should be in migrate, not migrate_drupal! Possibly related to the migrateDumpAlter() stuff introduced in #2417549: Drupal\migrate_drupal\Tests\d6\MigrateFileTest fail in MigrateTestBase, is the alter being called here? Maybe I should look at the actual patch...

mikeryan’s picture

Without looking carefully at each test altered to use the simplified interface (I can do that once the testbot is happy), this looks good - definitely a step forward for writing/reviewing tests. Just need to get that damned upload test squared away...

mikeryan’s picture

Using run-tests.sh locally I get

Fatal error: Call to a member function getFileUri() on a non-object in /Users/mryan/Sites/d8/core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php on line 98

Call Stack:
    0.0056     278176   1. {main}() /Users/mryan/Sites/d8/core/scripts/run-tests.sh:0
    2.2457   11338940   2. simpletest_script_run_one_test() /Users/mryan/Sites/d8/core/scripts/run-tests.sh:43
    2.2643   12061544   3. Drupal\simpletest\TestBase->run() /Users/mryan/Sites/d8/core/scripts/run-tests.sh:644
   13.5377   24856052   4. Drupal\migrate_drupal\Tests\d6\MigrateFileTest->testFiles() /Users/mryan/Sites/d8/core/modules/simpletest/src/TestBase.php:994
mikeryan’s picture

Status: Needs work » Needs review
FileSize
108.79 KB
1.48 KB

So, MIgrateTestBase::prepare() is no longer called, but that's where the migrateDumpAlter() was. I moved that to executeMigrate() and removed prepare() - local testing is happier, let's see how testbot feels.

benjy’s picture

As discussed in irc lets move executeMigration() into the parent.

phenaproxima’s picture

FileSize
109.29 KB
2.64 KB

Moved executeMigration() into MigrateTestBase, where it belongs.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2514168-10.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
109.39 KB

Re-rolled.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

No functionality was changed in the previous patch, so I'm OK kicking this back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

git ac https://www.drupal.org/files/issues/2514168-14.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  109k  100  109k    0     0  85449      0  0:00:01  0:00:01 --:--:-- 85510
error: patch failed: core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php:93
error: core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php: patch does not apply
error: patch failed: core/modules/migrate_drupal/src/Tests/d6/MigrateUserPictureFileTest.php:8
error: core/modules/migrate_drupal/src/Tests/d6/MigrateUserPictureFileTest.php: patch does not apply
phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
109.44 KB

Re-rolled. Due to the conflicts, interdiff decided to throw a big fat error in my face, so I apologize for the lack thereof.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
110.49 KB
907 bytes
benjy’s picture

Status: Needs review » Reviewed & tested by the community

Great

alexpott’s picture

Category: Bug report » Task
Status: Reviewed & tested by the community » Fixed

At some point you are probably going to want to stop refining things like this. Yes this is a nice tidy up but it does not make any functional change. Anyhow since the migrate maintainers want this in and it is test only change...

Committed 91ab3cc and pushed to 8.0.x. Thanks!

btw this is not fixing a bug afaics

  • alexpott committed 91ab3cc on 8.0.x
    Issue #2514168 by phenaproxima, mikeryan, benjy: Streamline...

Status: Fixed » Closed (fixed)

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