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.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff.txt | 713 bytes | quietone |
#17 | 2862006-17.patch | 2.74 KB | quietone |
#13 | interdiff-7-13.txt | 743 bytes | jofitz |
#13 | 2862006-13.patch | 1.83 KB | jofitz |
#7 | interdiff.txt | 558 bytes | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedRestore 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.
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedSo, 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
So, no rewind.
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedComment #7
quietone CreditAttribution: quietone as a volunteer commentedRight, 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.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedOops, went to let the cat out and didn't set to NR.
Comment #9
phenaproximaSelf-assigning for review.
Comment #10
heddnWe 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.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedMight as well add the blocker tag
Comment #12
phenaproximaWe 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.
Comment #13
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded the condition suggested by @phenaproxima and added to the comment to explain the change.
Comment #14
phenaproximaI think this is ready to go.
Comment #16
Gábor HojtsyLooks like a legitimate fail due to an unrelated patch commit:
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedOh, #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.
Comment #18
phenaproximaMakes sense.
Comment #21
Gábor HojtsyThis 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 :)