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.

CommentFileSizeAuthor
#35 interdiff-2791119-34-35.txt547 bytesphenaproxima
#35 2791119-35.patch13.38 KBphenaproxima
#34 interdiff-2791119-29-34.txt1.43 KBphenaproxima
#34 2791119-34.patch13.34 KBphenaproxima
#29 interdiff-2791119-27-29.txt1.53 KBphenaproxima
#29 2791119-29.patch13.28 KBphenaproxima
#27 interdiff-2791119-23-27.txt1.48 KBphenaproxima
#27 2791119-27.patch13.4 KBphenaproxima
#23 interdiff-2791119-19-23.txt491 bytesphenaproxima
#23 2791119-23.patch13.03 KBphenaproxima
#19 interdiff-2791119-15-19.txt1.86 KBphenaproxima
#19 2791119-19.patch13.01 KBphenaproxima
#15 interdiff-2791119-10-15.txt4.27 KBphenaproxima
#15 2791119-15.patch12.75 KBphenaproxima
#10 interdiff-2791119-7-10.txt2.34 KBphenaproxima
#10 2791119-10.patch12.87 KBphenaproxima
#7 interdiff-2791119-6-7.txt1.13 KBphenaproxima
#7 2791119-7.patch10.2 KBphenaproxima
#6 interdiff-2791119-0-6.txt2.36 KBphenaproxima
#6 2791119-6.patch10.86 KBphenaproxima
migrate-source-kernel-test-base.patch10.23 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
Issue tags: +DX (Developer Experience)
chx’s picture

We 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.

phenaproxima’s picture

I 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.

quietone’s picture

Wow, 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.

phenaproxima’s picture

A few very minor changes.

phenaproxima’s picture

A couple more tiny things.

mikeryan’s picture

Version: 8.3.x-dev » 8.2.x-dev

Nice! What's the path forward from this? A little weird to convert just one test... One followup (or several) to convert others?

phenaproxima’s picture

I converted the single test as proof that these classes work. The path forward, I think, is:

  1. Deprecate MigrateSqlSourceTestCase.
  2. Get this patch committed.
  3. Open a follow-up issue (possibly a meta-issue) to convert the existing tests to this.
phenaproxima’s picture

Added a deprecation notice for MigrateSqlSourceTestCase, and converted another simple test.

mikeryan’s picture

Can 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.

phenaproxima’s picture

We 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.

mikeryan’s picture

Sure, stick to the role test, keeping this patch small and simple will help get it in quicker.

quietone’s picture

+++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
@@ -0,0 +1,175 @@
+    /** @var MigrationInterface|\Prophecy\Prophecy\ObjectProphecy $migration */

Is 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.

phenaproxima’s picture

OK! Fixed the nit in #14 (good eye!) and removed the refactored CommentTest.

chx’s picture

Title: Make Migrate's source plugin tests great again » Write meaningful Migrate source tests finally
chx’s picture

Title: Write meaningful Migrate source tests finally » Write meaningful Migrate source tests
quietone’s picture

Just a few things.

  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,176 @@
    +      $this->fail('No plugin class was specified.');
    

    This has a full stop.

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,176 @@
    +    $this->fail('No plugin found for class ' . $class);
    

    And this doesn't. I can't find which one is preferred.

  3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,176 @@
    +   *   The plugin configuration.
    

    Elsewhere in this doc block the plugin is referred to as the source plugin. So s/plugin/source plugin/ ?

  4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
    @@ -0,0 +1,84 @@
    +   * @param int $expected_count
    +   *   (optional) How many rows the source plugin is expected to return.
    

    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?

phenaproxima’s picture

All 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.

quietone’s picture

Thx!

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Free to proceed as far as I'm concerned.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2791119-19.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
13.03 KB
491 bytes

Heh...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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Nice. Back to RTBC.

xjm’s picture

Issue tags: +rc eligible

As a test improvement to an experimental module, this is rc eligible per https://www.drupal.org/core/d8-allowed-changes#rc.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,181 @@
    +   * @see testSource for the elements that should be included in each data set
    +   * returned by this method.
    +   *
    +   * @return array
    

    This 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.

    FILE: .../core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
     37 | ERROR | The @see reference should not contain any additional
        |       | text
    ----------------------------------------------------------------------
    
  2. +++ b/core/modules/user/tests/src/Kernel/Plugin/migrate/source/d7/RoleTest.php
    @@ -0,0 +1,72 @@
    +class RoleTest extends MigrateSqlSourceTestBase  {
    
    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
     13 | ERROR | [x] Expected 1 space before opening brace; found 2
    ----------------------------------------------------------------------
    PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    ----------------------------------------------------------------------
    
phenaproxima’s picture

benjy’s picture

  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,190 @@
    + * Base class for unit tests of Migrate source plugins.
    

    No longer a unit test.

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,190 @@
    +    $this->plugin = NULL;
    

    Why is this needed? State isn't maintained between runs?

  3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,190 @@
    +    $class = ltrim($this->getPluginClass(), '\\');
    ...
    +      if (ltrim($definition['class'], '\\') == $class) {
    

    Why do we trim the class and then trim the definition class we're comparing against? Are they sometimes different?

  4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,190 @@
    +    /** @var Row $row */
    

    Full path to Row.

  5. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
    @@ -0,0 +1,84 @@
    + * Base class for unit tests of Migrate source plugins that use a database.
    

    No longer unit tests.

  6. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
    @@ -0,0 +1,84 @@
    +  protected function getDatabase(array $source_data) {
    

    Why can't we use the database connection the test is using?

phenaproxima’s picture

  1. Fixed.
  2. Fixed.
  3. To keep things consistent. It's defensive -- it's possible that the plugin definition's class path will have the leading slash, but the @covers annotation will not.
  4. Fixed.
  5. Fixed.
  6. I heartily agree, but that's out of scope here. We should address that in a follow-up issue.
benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

phenaproxima’s picture

Issue tags: +Dublin2016
chx’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,182 @@
    +   * Each data set should consist of the following elements:
    +   *
    +   *  - An array of source data, which can be optionally processed and set up
    +   *    by subclasses.
    +   *  - An array of expected result rows.
    +   *  - (optional) The number of result rows the plugin under test is expected
    +   *    to return. If this is not a numeric value, the plugin will not be
    +   *    counted.
    +   *  - (optional) Array of configuration options for the plugin under test.
    ...
    +   * @return array
    

    This can go into the @return docs.

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
    @@ -0,0 +1,182 @@
    +   * @see testSource
    

    This should be be fully qualified.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
13.34 KB
1.43 KB
phenaproxima’s picture

D'oh, didn't realize what "fully qualified" meant.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Great

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 27b61a9 to 8.3.x and abc606f to 8.2.x. Thanks!

  • alexpott committed 27b61a9 on 8.3.x
    Issue #2791119 by phenaproxima, quietone, chx, mikeryan, benjy: Write...

  • alexpott committed abc606f on 8.2.x
    Issue #2791119 by phenaproxima, quietone, chx, mikeryan, benjy: Write...

Status: Fixed » Closed (fixed)

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