Migrate's source plugin tests are currently kinda...not good.
For one thing, they are all unit tests. This is because that was the fastest way to satisfy the pointless "every patch must have a test" even when there's nothing to test on the majority of source plugins. It's the equivalent of writing "Interface for context" on ContextInterface to check a checkbox (except of course context does need documentation unlike source plugins where the majority still doesn't need a test).
Another problem is that, given the way they're structured, you can only test a single complete data set using a single set of plugin configuration. It's difficult to write very extensive test coverage if, say, you want to test the same plugin under different configurations. In fact, most of the time you need to write a completely new test class for that. Given how configurable source plugins can be, that's far from ideal.
This patch introduces a new pair of base test classes, extending KernelTestBase, for writing better and more comprehensive tests of source plugins. Because they are kernel tests, mocking services is unncessary -- only the migration itself is mocked (prophesized, actually). These classes are built around PHPUnit's dataProvider pattern, which makes it possible to easily test the same plugin many times with radically different data sets and configuration.
Because this introduces completely new test classes instead of altering existing ones, this patch maintains BC and would allow us to migrate the existing source plugin tests to it on a continuing basis. I've ported a single test -- for the d7_user_role plugin -- as proof of concept.
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff-2791119-34-35.txt | 547 bytes | phenaproxima |
#35 | 2791119-35.patch | 13.38 KB | phenaproxima |
#34 | interdiff-2791119-29-34.txt | 1.43 KB | phenaproxima |
#34 | 2791119-34.patch | 13.34 KB | phenaproxima |
#29 | interdiff-2791119-27-29.txt | 1.53 KB | phenaproxima |
Comments
Comment #2
phenaproximaComment #3
chx CreditAttribution: chx at Smartsheet, Migrate Rocks commentedWe should nuke the existing unit tests, they are useless and were written just to get migrate into core, they are the equivalent of "interface for context" doxygen on ContextInterface.
Comment #4
phenaproximaI don't agree that they are completely useless. Some source plugins perform additional processing on the raw data they retrieve, and the tests are useful for verifying the results of those transformations. I do feel that having universe-mocking unit tests decreases their usefulness, because if you can control the environment completely, you reduce the chances of discovering real bugs. That's another advantage to making them into kernel tests.
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedWow, this is sooo much easier to work with. I was eager to try this out by reworking the test for the d8 config source plugin. That one became messy when I added different plugin configurations. So I did. And it was great. No complaints with using it at all.
thx, phenapromixa! It is always nice to end the day on a positive note.
Comment #6
phenaproximaA few very minor changes.
Comment #7
phenaproximaA couple more tiny things.
Comment #8
mikeryanNice! What's the path forward from this? A little weird to convert just one test... One followup (or several) to convert others?
Comment #9
phenaproximaI converted the single test as proof that these classes work. The path forward, I think, is:
Comment #10
phenaproximaAdded a deprecation notice for MigrateSqlSourceTestCase, and converted another simple test.
Comment #11
mikeryanCan we remove the old role and comment tests being obsoleted here? No BC reason to keep them, I think, and having the old and new tests side-by-side in the patch will help demonstrate the utility of the new approach.
Comment #12
phenaproximaWe can remove the old Role test, but not the comment test (yet) because it's being extended by a test of high-water support, which is totally broken at the moment. Once #2485385: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface is in, both comment tests can be removed because at that point we'll be able to test the high-water stuff properly on top of MigrateSqlSourceTestBase. That said, maybe I should remove the port of CommentTest until that happens.
Comment #13
mikeryanSure, stick to the role test, keeping this patch small and simple will help get it in quicker.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedIs this needed? migration is already declared?
+1 RTBC because it works so well. But since there are things in the code I'm still learning I'm leaving as NR.
Comment #15
phenaproximaOK! Fixed the nit in #14 (good eye!) and removed the refactored CommentTest.
Comment #16
chx CreditAttribution: chx at Smartsheet, Migrate Rocks commentedComment #17
chx CreditAttribution: chx at Smartsheet, Migrate Rocks commentedComment #18
quietone CreditAttribution: quietone as a volunteer commentedJust a few things.
This has a full stop.
And this doesn't. I can't find which one is preferred.
Elsewhere in this doc block the plugin is referred to as the source plugin. So s/plugin/source plugin/ ?
Making the assertion of the count optional is different. So now, to use this the expected count has to be added manually whereas before it was automatic. Is there a reason for this change? Can the previous behavior be restored?
Comment #19
phenaproximaAll fixed.
Regarding #18.3, I had made $expected_count manual because back when I was working full-time on Migrate I got burned once or twice by MigrateSqlSourceTestCase's sledgehammer insistence on counting the plugin. That said, you're completely right, the default behavior should be to count it -- but we must also be able to override that. So that is what I have done. If $expected_count is not passed in, the plugin will be counted. Otherwise, if it's a non-numeric value of any kind, it will be skipped.
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedThx!
Comment #21
mikeryanFree to proceed as far as I'm concerned.
Comment #23
phenaproximaHeh...now that #2560795: Source plugins have a hidden dependency on migrate_drupal is in, a source plugin test that requires Migrate Drupal needs to actually mention Migrate Drupal in its $modules property. Good.
Comment #24
phenaproximaNice. Back to RTBC.
Comment #25
xjmAs a test improvement to an experimental module, this is rc eligible per https://www.drupal.org/core/d8-allowed-changes#rc.
Comment #26
alexpottThis needs better documentation to help people use this base test class as it is the extension point. Plus using @see like this is a violation of our coding standards.
Comment #27
phenaproximaBoth fixed.
Comment #28
benjy CreditAttribution: benjy at PreviousNext commentedNo longer a unit test.
Why is this needed? State isn't maintained between runs?
Why do we trim the class and then trim the definition class we're comparing against? Are they sometimes different?
Full path to Row.
No longer unit tests.
Why can't we use the database connection the test is using?
Comment #29
phenaproximaComment #30
benjy CreditAttribution: benjy at PreviousNext commentedLooks good to me.
Comment #31
phenaproximaComment #32
chx CreditAttribution: chx at Smartsheet, Migrate Rocks commentedComment #33
alexpottThis can go into the @return docs.
This should be be fully qualified.
Comment #34
phenaproximaComment #35
phenaproximaD'oh, didn't realize what "fully qualified" meant.
Comment #36
benjy CreditAttribution: benjy at PreviousNext commentedGreat
Comment #37
alexpottCommitted and pushed 27b61a9 to 8.3.x and abc606f to 8.2.x. Thanks!