#2560671: The Migration process plugin should not skip rows removes the unit test for the Migration process plugin. This is a good thing, because it only had one test, which asserted that the plugin would throw MigrateSkipRowException when lookups failed. Since that no longer happens (as of that issue), the test was rendered irrelevant.

However, it's not OK for the Migration plugin, of all things, to have no test coverage. It's a complex and crucial plugin. It needs tests that actually test it thoroughly. Let's add some.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Postponed » Active

Unblocked.

phenaproxima’s picture

It's a start.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Status: Active » Needs work

Well, the unit MigrationTest.php has been created, but is only testing stubbing stuff - the general functionality test here should be merged into it.

quietone’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

OK. Merged the code in #3.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

This has been sorely needed for a while. Great stuff.

The last submitted patch, 3: 2561049-3.patch, failed testing.

chx’s picture

tl; dr: be careful with the word "proper" in the issue title because I might hold it against you :)

This is great work to start with. The question which I have no answers to where do we want to stop? There are a healthy number of different scenarios which could be tested here. Do we want to? I would love to see more testing but is it worth the work? I do not know. In particular, scalar vs non scalar and then one destination id and more than one destination id -- that's four cases. Independently you can specify a single migration or several ones (while more rare the support is there, for example, if you are migrating something that wants to look up nodes in general then you can specify every node type migration in one go) . Then there's the no_stub option for fail and non fail cases. Then you need to look at stubbing. Provide four scenarios: two with multiple migrations, of these one where self migration is used to stub the other when explicit stub_id is given or simply there's just one migration and finally when multiple migrations are given and so stubbing doesn't happen because the system doesn't know where to stub.

phenaproxima’s picture

My feeling is that I'd like to get some sort of dedicated unit test(s) for the Migration plugin into core, even if we don't cover its myriad possible use cases. As you point out, there are so many that we'd be here for a year if we tried to cover them all. We know the plugin works quite well, since it's used extensively in the core upgrade path and has rarely caused trouble. But it's unacceptable for it to have NO dedicated tests at all. So IMHO this is a great start, and I think we can add more test coverage later as needed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is a good starter. Committed and pushed 426c9fd32d9601dd7118a60ffebaaff51f1a74f5 to 8.2.x and d30cc2e to 8.1.x. Thanks!

  • alexpott committed 426c9fd on 8.2.x
    Issue #2561049 by phenaproxima, quietone: Add proper unit tests for the...

  • alexpott committed d30cc2e on 8.1.x
    Issue #2561049 by phenaproxima, quietone: Add proper unit tests for the...

Status: Fixed » Closed (fixed)

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