While working on the source plugin test for the migration of d6 menu links translations, I found that the test was returning false positives. The check on the expected number of rows works as expected but the assertions on the row content is completely skipped.

In \Drupal\Tests\migrate\Kernel\MigrateSourceTestBase::testSource the following foreach in not entered for MenuLinkTest.php or MenuLinkTranslationTest.php (my local version). Yet, that loop is entered for UserPictureInstanceTest.php, which is just a test I checked to see if this effects all tests. So, some tests are effected but exactly which ones I don't know.

    $i = 0;
    /** @var \Drupal\migrate\Row $row */
    foreach ($plugin as $row) {
      $this->assertInstanceOf(Row::class, $row);

Further testing and I tracked the cause to the highwater patch, #2824267: Highwater condition isn't added (on remote databases). If I reverse that patch, the loop shown above is entered and the test works as expected.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Restore the original count method that got changed in #2824267: Highwater condition isn't added (on remote databases) (yea, by me) and a test to confirm that the foreach loop was entered during. Also, a fail patch.

The last submitted patch, 2: 2862006-1-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2862006-1.patch, failed testing.

quietone’s picture

So, 58 source plugin tests are false positives and there are 72 source plugins.

Not surprisingly, the failure on the patch is from HighWaterNotJoinableTest.php. InitializeIterator is not executed so the highwater condition is not added and the plugin returns 3 rows are returned instead of the desired 2. That is easily fixed by using iterator_count.

But then the problem is that after the count the position of the iterator can't be guaranteed. So, the next test the foreach fails because the iterator is 'messed up'. But wait, the foreach does a rewind() so it should be good to go. Ah,not really, that rewind goes to

  public function rewind() {
    // Nothing to do: our DatabaseStatement can't be rewound.
  }

So, no rewind.

quietone’s picture

Title: MigrateSourceTestBase gives false positives for MenuLinkTest » MigrateSourceTestBase returns false positives for most plugin tests
quietone’s picture

Right, lets just adjust the expected count to reflect what count is really happening here, the total number of available rows, before the query is modified. The foreach loop will still run and assert that the expected data is correct. If the plugin returns more rows than expected the test fails with an 'undefined offset' error because it is trying to access a row in the expected_data array that does not exist. If the plugin returns less rows than expected than the test will fail on assertCount.

A potential problem exists because of testing that the foreach loop was entered. That means that the source plugin under test must return at least one row for the test. A test case where the plugin returns zero rows will always fail.

quietone’s picture

Status: Needs work » Needs review

Oops, went to let the cat out and didn't set to NR.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

heddn’s picture

We discussed this in our weekly maintainers meeting and decided this is indeed important. It is blocking any new changes to source plugins and blocks all i18n work.

quietone’s picture

Issue tags: +blocker

Might as well add the blocker tag

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
@@ -185,6 +185,9 @@ public function testSource(array $source_data, array $expected_data, $expected_c
+    // False positives occur if the foreach is not entered. So, confirm the
+    // foreach loop was entered.
+    $this->assertGreaterThan(0, $i);

We need to wrap this in an if ($expected_count > 0) check or something, or it won't be possible to test for an empty result set (which will never enter the foreach loop).

Otherwise, looks perfect.

jofitz’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
743 bytes

Added the condition suggested by @phenaproxima and added to the comment to explain the change.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2862006-13.patch, failed testing.

Gábor Hojtsy’s picture

Looks like a legitimate fail due to an unrelated patch commit:

1) Drupal\Tests\comment\Kernel\Plugin\migrate\source\d6\CommentSourceWithHighWaterTest::testSource with data set #0 (array(array(array(1, 0, 2, 3, 'subject value 1', 'comment value 1', 'hostname value 1', 1382255613, 0, '', '', '', '', 'testformat1', 'story'), array(2, 1, 3, 4, 'subject value 2', 'comment value 2', 'hostname value 2', 1382255662, 0, '', '', '', '', 'testformat2', 'page')), array(array(2, 'story'), array(3, 'page'))), array(array(2, 1, 3, 4, 'subject value 2', 'comment value 2', 'hostname value 2', 1382255662, 1, '', '', '', '', 'testformat2', 'page')), null, array(array('timestamp')), 1382255613)
Failed asserting that actual size 2 matches expected size 1.

/var/www/html/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php:164
/var/www/html/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php:83
quietone’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
713 bytes

Oh, #2807875: Convert Comment's Migrate source tests to new base class modifies source plugins and should have been postponed on this issue. Ideally, this issue would have been committed first and then #2807875: Convert Comment's Migrate source tests to new base class rerolled to update CommentSourceWithHighWaterTest.php. Obviously, that didn't happen.

I think the simplest thing to do is to correct the Comment test here. It does make sense in that the source plugin test did need to be modified to handle high water correctly.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense.

  • Gábor Hojtsy committed 1b9d416 on 8.3.x
    Issue #2862006 by quietone, Jo Fitzgerald, phenaproxima:...

  • Gábor Hojtsy committed 9ae4d12 on 8.4.x
    Issue #2862006 by quietone, Jo Fitzgerald, phenaproxima:...
Gábor Hojtsy’s picture

Assigned: phenaproxima » Unassigned
Status: Reviewed & tested by the community » Fixed

This indeed rolls back the \Countable => \Iterator change from #2824267: Highwater condition isn't added (on remote databases), the fix makes sense and the resulting assorted fail fixes also look good. Yay for unblocking further migrate work :)

Status: Fixed » Closed (fixed)

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