Problem/Motivation

Over in #2533886: [meta] Move module-specific migration support into the particular modules supported, we've decided to split the Drupal 6 and 7 migration paths into their constituent modules. It's pretty straightforward, but the work is breaking MigrateDrupal6Test (an integration test of a full Drupal 6 site migration) because this test relies on a hard-coded laundry list of test classes.

Proposed Resolution

MigrateFullDrupalTestBase (the parent class of MigrateDrupal6Test) should discover tests dynamically, using the test_discovery service. This will fix the problem and ensure that we can easily add more integration tests without having to alter MigrateDrupal6Base (or its eventual Drupal 7 counterpart).

Remaining Tasks

Review the patch and commit.

API Changes

None.

UI Changes

None.

Comments

phenaproxima’s picture

Status: Active » Needs review
StatusFileSize
new6.86 KB
mikeryan’s picture

mikeryan’s picture

Nice, I'll be happy to rtbc once testbot approves.

phenaproxima’s picture

Issue tags: +blocker, +Migrate critical

Status: Needs review » Needs work

The last submitted patch, 1: 2534158-1.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review

One of those testbot race condition things?

mikeryan queued 1: 2534158-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2534158-1.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new8.54 KB
new2.53 KB

Fixed, but I'm sorry to say it involved an ugly, though wholly necessary (albeit temporary, thankfully) kludge. I left a detailed comment in MigrateFullDrupalTestBase -- see the interdiff. It's a tricky little issue, and I couldn't see any other way out of it.

I don't think we'll have any choice but to commit this to core with the hack for now. Once #2533886: [meta] Move module-specific migration support into the particular modules supported lands we can remove it in a follow-up issue.

mikeryan’s picture

QQ (without looking carefully at the tests in question) - are all tests that need to be run via MigrateFullDrupalTestBase destined to move to other modules, or might there be some outliers that need to remain in migrate_drupal? And if there are outliers, would they prevent us from removing the hack?

phenaproxima’s picture

And if there are outliers, would they prevent us from removing the hack?

In a word: yes. I thought about that and what I think might be a good solution is that, if there are outliers, we should simply change their group to migrate_drupal_6 and migrate_drupal_7, as appropriate, and filter on that instead of testing the class name against a regex.

There was an issue open to change the existing tests to use those groups, but that patch was big and sweeping and almost entirely obviated by #2533886: [meta] Move module-specific migration support into the particular modules supported, so I closed it. I didn't want to let it block this issue. So I'm thinking we can commit this with the hack for now, and when the dust settles in the meta-issue, we can revisit this if any outliers remain.

phenaproxima’s picture

StatusFileSize
new7.39 KB
new3.14 KB

At @mikeryan's suggestion, created a blacklist array in MigrateFullDrupalTestBase, rather than an awkward namespace-checking hack. Seems to solve the problem...and it's a much better way to go about fixing the trouble I found in #9.

chx’s picture

Status: Needs review » Reviewed & tested by the community

We discussed this and determined that the blacklist functionality does not need to be extensible since the one blacklisted is an API-ish test (as much as migrate drupal has an API) and contrib will actually tests that test migrations.

mikeryan’s picture

+1 on the RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Migrate is not subject to beta evaluation. Committed 978fcf0 and pushed to 8.0.x. Thanks!

  • alexpott committed 978fcf0 on 8.0.x
    Issue #2534158 by phenaproxima, mikeryan: MigrateFullDrupalTestBase must...

Status: Fixed » Closed (fixed)

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