Convert Comment's Migrate source tests to new base class

I would add here what alexpott suggested in order to help to review then commit it:
See #2807879-14: Convert Contact's Migrate source tests to new base class - let's use better array in/out keys to make it easier to understand what is going on.

Adding d6/CommentTest, I don't know how to set the high water, so still to do d6/CommentSourceWithHighWaterTest.php, any ideas?

I think that adding high-water support will involve changing the MigrateSourceTestBase class -- which is totally fine. :) I'd recommend adding a new parameter to testSource(), call it $high_water_mark or similar, and default it to NULL. Then, in that method, if $high_water_mark is set, set the value in the high-water storage service (which is referenced in SourcePluginBase::getHighWaterStorage(), I believe). This should all be done before iterating over the source to assert the results.

Something like this should do the trick, before the $expected_count is asserted:

    // If there is a high-water mark, set it in the high-water storage.
    if (isset($high_water_mark)) {
      $this->container
        ->get('keyvalue')
        ->get('migrate:high_water')
        ->set($this->migration->id(), $high_water_mark);
    }

Heh, I just realized my mistake -- try changing this:

$this->container
        ->get('keyvalue')
        ->get('migrate:high_water')
        ->set($this->migration->id(), $high_water_mark);

...to this:

$this->container
        ->get('keyvalue')
        ->get('migrate:high_water')
        ->set($this->migration->reveal()->id(), $high_water_mark);

Thanks for the nod, @quietone.

Highwater issue is in, unpostponing and testing latest patch.

Re-rolled.

Helps if I roll it against an up-to-date codebase...

+++ b/core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
@@ -158,8 +158,8 @@ public function testSource(array $source_data, array $expected_data, $expected_c
+    // countable and there is no high water mark.
+    if (is_numeric($expected_count) && !isset($high_water)) {
       $this->assertInstanceOf('\Iterator', $plugin);
       $this->assertSame($expected_count, iterator_count($plugin));
     }
@@ -185,6 +185,10 @@ public function testSource(array $source_data, array $expected_data, $expected_c

@@ -185,6 +185,10 @@ public function testSource(array $source_data, array $expected_data, $expected_c
         }
       }
     }
+
+    if (isset($high_water)) {
+      $this->assertSame($expected_count, $i);
+    }

I find this stuff a little strange. Firstly, won't the expected count be influenced by the high-water value? Why would we only want to assert the count if there's no high-water mark?

Second, I'm a bit uncomfortable asserting anything against $i -- it's just a counter value to loop through the expected data and probably shouldn't be treated as any kind of canonical value to assert against.

Everything else was great. Proceed!

Committed bdcd497 and pushed to 8.4.x. Thanks!

I think we should backport this to 8.3.x to keep everything the same in test land.

+++ /dev/null
@@ -1,80 +0,0 @@
-abstract class CommentTestBase extends MigrateSqlSourceTestCase {

Experimental - this is okay.

Committed 102cffe and pushed to 8.3.x. Thanks!