#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.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 2561049-6.patch | 3.78 KB | quietone |
| #3 | 2561049-3.patch | 3.39 KB | phenaproxima |
Comments
Comment #2
phenaproximaUnblocked.
Comment #3
phenaproximaIt's a start.
Comment #5
mikeryanWell, the unit MigrationTest.php has been created, but is only testing stubbing stuff - the general functionality test here should be merged into it.
Comment #6
quietone commentedOK. Merged the code in #3.
Comment #7
phenaproximaThis has been sorely needed for a while. Great stuff.
Comment #9
chx commentedtl; 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.
Comment #10
phenaproximaMy 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.
Comment #11
alexpottThis is a good starter. Committed and pushed 426c9fd32d9601dd7118a60ffebaaff51f1a74f5 to 8.2.x and d30cc2e to 8.1.x. Thanks!