Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff-2514168-17-19.txt | 907 bytes | phenaproxima |
#19 | 2514168-19.patch | 110.49 KB | phenaproxima |
#17 | 2514168-17.patch | 109.44 KB | phenaproxima |
#14 | 2514168-14.patch | 109.39 KB | phenaproxima |
#11 | interdiff-2514168-9-10.txt | 2.64 KB | phenaproxima |
Comments
Comment #2
phenaproximaMissed a few spots. Danke, testbot.
Comment #4
phenaproximaD'oh. Let's play again!
Comment #6
mikeryanOoh, 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...
Comment #7
mikeryanWithout 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...
Comment #8
mikeryanUsing run-tests.sh locally I get
Comment #9
mikeryanSo, 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.
Comment #10
benjy CreditAttribution: benjy at CodeDrop commentedAs discussed in irc lets move executeMigration() into the parent.
Comment #11
phenaproximaMoved executeMigration() into MigrateTestBase, where it belongs.
Comment #12
benjy CreditAttribution: benjy at CodeDrop commentedLooks good.
Comment #14
phenaproximaRe-rolled.
Comment #15
phenaproximaNo functionality was changed in the previous patch, so I'm OK kicking this back to RTBC.
Comment #16
alexpottNeeds a reroll...
Comment #17
phenaproximaRe-rolled. Due to the conflicts, interdiff decided to throw a big fat error in my face, so I apologize for the lack thereof.
Comment #18
phenaproximaComment #19
phenaproximaRe-rolled again, now that #2409415: Variable to config: aggregator.settings [d7] landed.
Comment #20
benjy CreditAttribution: benjy at CodeDrop commentedGreat
Comment #21
alexpottAt 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