Convert Migrate_drupal's Migrate source tests to new base class

Hi @quietone,
I was reviewing and testing your patch and it looks great to me.
I have only one suggestion : Use only one way to declare an array. In the class i18nVariableTest, you have used two different ways: array() and []. How you use [] more frequently in other parts of your code, I have replaced array() by [] in the patch 2807913-4.patch .

All tests pass for the patch 2807913-4.patch, @quietone and I have tested and review this.

@quietone
I found that replace only the way that arrays are declared was really a minor change, but you are completely right, is better if someone other takes a look. I have added the interdiff-2807913-3-4.txt to make this helpful as you suggested. Thanks!

The patch doesn't seem to convert the VariableMultiRow plugin tests.
Attached is an updated patch that converts this Plugin test as well.

@svendecabooter Thanks for your work! I have made minor changes to your patch to make it pass the tests.

Thanks erozqba!
Seems my "phpcbf" command added a dot at the end of the @covers statement, which is not really helpful :)

Assigning to myself for review.

Nothing objectionable here. Great work, guys.

1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/VariableMultiRowTest.php
   @@ -0,0 +1,41 @@
   +class VariableMultiRowTest extends VariableTest {
   ...
   +    // Use the same test data as parent.
   +    $tests = parent::providerSource();
   

   I think entangling these tests actually makes it harder to understand what is being tested. Let's just copy the providerSource from VariableTest and extend MigrateSqlSourceTestBase.

2. +++ /dev/null
   @@ -1,20 +0,0 @@
   -<?php
   -
   -namespace Drupal\Tests\migrate_drupal\Unit\source;
   -
   -/**
   - * Tests variable multirow source w/ high water handling.
   - *
   - * @group migrate_drupal
   - */
   -class VariableMultiRowSourceWithHighwaterTest extends VariableMultiRowTestBase {
   -
   -  /**
   -   * {@inheritdoc}
   -   */
   -  protected function setUp() {
   -    $this->migrationConfiguration['highWaterProperty']['field'] = 'test';
   -    parent::setup();
   -  }
   -
   -}
   

   Where's this test gone?

Hi @alexpott I don't know how to implement the VariableMultiRowSourceWithHighwaterTest, any hint you could provide here, any example? Thanks in advance.

1. VariableMultiRowTest now extends MigrateSqlSourceTestBase.
2. Re-instated VariableMultiRowSourceWithHighwaterTest.

(and replaced a few commas)

Correctly named the VariableMultiRowSourceWithHighwaterTest class(!) #facepalm

Tests pass after retest.

Great stuff. Thank you, @Jo Fitzgerald! I have only one concern about one of the tests, and it's nothing you did, but I'd like to address it here:

+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/VariableMultiRowSourceWithHighwaterTest.php
@@ -0,0 +1,58 @@
+    // The source plugin configuration.
+    $tests[0]['configuration']['variables'] = [
+      'foo',
+      'bar',
+    ];
+    $tests[0]['configuration']['high_water_property']['name'] = 'name';

I don't understand what this test is asserting. How can variables have a high-water mark? High-water marks are only for things that have a deterministic order; variables don't really fall into that category.

If this is what the old version of the test did, that old test is weird and strange and proves nothing, and we should drop this entire test class from our suite.

Werd to that. Looks poifect to me!

Any further condensing of these tests (i.e., taking greater advantage of the data provider pattern) can happen in a follow-up. Just moving these to the new base class is a big help. So let's get this in.

Testbot hiccup

Committed and pushed b523b90 to 8.4.x and b400bf8 to 8.3.x. Thanks!

Backported to 8.3.x as this is a tests only change to an experimental module.