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.

CommentFileSizeAuthor
#19 interdiff-2514168-17-19.txt907 bytesphenaproxima
#19 2514168-19.patch110.49 KBphenaproxima
#17 2514168-17.patch109.44 KBphenaproxima
#14 2514168-14.patch109.39 KBphenaproxima
#11 interdiff-2514168-9-10.txt2.64 KBphenaproxima
#11 2514168-10.patch109.29 KBphenaproxima
#9 interdiff.txt1.48 KBmikeryan
#9 2514168-9.patch108.79 KBmikeryan
#4 interdiff-2514168-2-4.txt756 bytesphenaproxima
#4 2514168-4.patch108.08 KBphenaproxima
#2 interdiff-2514168-0-2.txt9.1 KBphenaproxima
#2 2514168-2.patch108.08 KBphenaproxima
migrate_drupal-clean_tests.patch103 KBphenaproxima

Comments

Status: Needs review » Needs work

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2514168-2.patch108.08 KB
new interdiff-2514168-0-2.txt9.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

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2514168-4.patch108.08 KB
new interdiff-2514168-2-4.txt756 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

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

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

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

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

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

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

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2514168-9.patch108.79 KB
new interdiff.txt1.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

benjy commented

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2514168-10.patch109.29 KB
new interdiff-2514168-9-10.txt2.64 KB

Moved executeMigration() into MigrateTestBase, where it belongs.

benjy’s picture

benjy commented
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

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2514168-14.patch109.39 KB

Re-rolled.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2514168-17.patch109.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

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

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2514168-19.patch110.49 KB
new interdiff-2514168-17-19.txt907 bytes

Re-rolled again, now that #2409415: Variable to config: aggregator.settings [d7] landed.

benjy’s picture

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

Great

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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.