The migrate module is still considered an experimental module. As a result, its code and tests have been reviewed less than other more mature code. Attach are changes noted during a review of the tests in core/modules/migrate/tests/src/Unit/*.php. These changes include adding docblocks, missing @param/@var/@return descriptions and a few coding style changes.

CommentFileSizeAuthor
#25 2621486-25-migrate-tests-src-unit-d8.patch29.68 KBlars toomre
#22 2621486-21-migrate-tests-src-unit-d8.patch30.39 KBlars toomre
#19 2621486-19-migrate-tests-src-unit-d8.patch31.22 KBlars toomre
#19 interdiff-2621486-19-17.txt1.5 KBlars toomre
#17 2621486-17-migrate-test-src-unit-d8.patch30.9 KBlars toomre
#13 2621486-13-migrate-tests-src-unit-d8.patch28.56 KBlars toomre
#11 2621486-10-migrate-tests-src-unit-d8.patch31.23 KBlars toomre
#8 2621486-8-migrate-tests-src-unit-d8.patch31.23 KBlars toomre
#5 2621486-5-migrate-tests-src-unit-d8.patch32.35 KBlars toomre
#2 2621486-2-migrate-tests-src-unit-d8.patch32.67 KBlars toomre
#2 2621486-2-phpcs-before-after.txt30.35 KBlars toomre

Comments

Lars Toomre created an issue. See original summary.

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new 2621486-2-phpcs-before-after.txt30.35 KB
new 2621486-2-migrate-tests-src-unit-d8.patch32.67 KB

Attached is an initial patch with changes to core/modules/migrate/tests/src/Unit/*.php to bring them more in compliance with Drupal's coding and documentation standards. This patch has been generated with -U6 switch to give more context to the docblock additions/changes that require review of the method call definition.

Attached also is a text file summarizing the results of running the Drupal standard PHP_CodeSniffer before this patch is applied and the results afterwards. The intent is to also create issues against the Coder module to cover various errors encountered as well as for missing and/or inconsistent sniffs encountered while generating this patch.

Status: Needs review » Needs work

The last submitted patch, 2: 2621486-2-migrate-tests-src-unit-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented

On my current development computer, I do not have a testing environment set up. Hence, I cannot run the test suite locally and must rely on the test bot.

From the error report generated by the test bot, I am at a loss on what the problem with the patch in #2 might be. Can someone offer some guidance? Thanks in advance for your assistance.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2621486-5-migrate-tests-src-unit-d8.patch32.35 KB

As I explained in #4, I am unsure what the problem with the patch in #2 is. The error messages from the test bot are not that helpful so I am wondering if somehow I changed the line endings to Windows format. Here is the patch again with each file explicitly set to Unix line endings. Let's see what the test bot says.

Status: Needs review » Needs work

The last submitted patch, 5: 2621486-5-migrate-tests-src-unit-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Related issues: +#2624558: Create sniff for missing @param directive when other @param tags exist

Created #2624558: Create sniff for missing @param directive when other @param tags exist to add a sniff for the missing final @param directive in the constructor method of TestSqlIdMap.php

lars toomre’s picture

lars toomre commented
StatusFileSize
new 2621486-8-migrate-tests-src-unit-d8.patch31.23 KB

Let's try to correct the final two test files. I was glad to see the number of errors drop from 142 to 4.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review

Whoops. Forget to change the status to Needs review.

Status: Needs review » Needs work

The last submitted patch, 8: 2621486-8-migrate-tests-src-unit-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2621486-10-migrate-tests-src-unit-d8.patch31.23 KB

I am really puzzled by why MigrateExecutableTest.php has failed. The only changes to that file are documentation changes. I have saved that file again with Unix line endings so let see what the bot thinks this time.

Status: Needs review » Needs work

The last submitted patch, 11: 2621486-10-migrate-tests-src-unit-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2621486-13-migrate-tests-src-unit-d8.patch28.56 KB

Reverted the changes to MigrateExecutableTest.php. I am hoping this version of the patch now passes the test bot.

Status: Needs review » Needs work

The last submitted patch, 13: 2621486-13-migrate-tests-src-unit-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented

Very puzzling... The patch in #13 does not include any changes to MigrateExecutableTest.php and yet it is that unit test that is failing just like in #11. Anyone have an idea what is going on?

quietone’s picture

quietone commented

Lars Toomre, thx for your fixes. I've found them quite helpful.

+++ b/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php
@@ -42,21 +42,21 @@ public function setStringTranslation(TranslationInterface $string_translation) {
+  public function setSource(MigrateSourceInterface $source) {

Looks like the error is caused by adding the type hint.

Here the error output:

PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

.EEEEEE..

Time: 231 ms, Memory: 10.00Mb

There were 6 errors:

1) Drupal\Tests\migrate\Unit\MigrateExecutableTest::testImportWithValidRow
Argument 1 passed to Drupal\Tests\migrate\Unit\TestMigrateExecutable::setSource() must be an instance of Drupal\Tests\migrate\Unit\MigrateSourceInterface, instance of Mock_SourcePluginBase_debde174 given, called in /opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php on line 105 and defined

/opt/sites/md82/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php:48
/opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php:105

2) Drupal\Tests\migrate\Unit\MigrateExecutableTest::testImportWithValidRowWithoutDestinationId
Argument 1 passed to Drupal\Tests\migrate\Unit\TestMigrateExecutable::setSource() must be an instance of Drupal\Tests\migrate\Unit\MigrateSourceInterface, instance of Mock_SourcePluginBase_debde174 given, called in /opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php on line 147 and defined

/opt/sites/md82/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php:48
/opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php:147

3) Drupal\Tests\migrate\Unit\MigrateExecutableTest::testImportWithValidRowNoDestinationValues
Argument 1 passed to Drupal\Tests\migrate\Unit\TestMigrateExecutable::setSource() must be an instance of Drupal\Tests\migrate\Unit\MigrateSourceInterface, instance of Mock_SourcePluginBase_debde174 given, called in /opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php on line 187 and defined

/opt/sites/md82/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php:48
/opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php:187

4) Drupal\Tests\migrate\Unit\MigrateExecutableTest::testImportWithValidRowWithDestinationMigrateException
Argument 1 passed to Drupal\Tests\migrate\Unit\TestMigrateExecutable::setSource() must be an instance of Drupal\Tests\migrate\Unit\MigrateSourceInterface, instance of Mock_SourcePluginBase_debde174 given, called in /opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php on line 246 and defined

/opt/sites/md82/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php:48
/opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php:246

5) Drupal\Tests\migrate\Unit\MigrateExecutableTest::testImportWithValidRowWithProcesMigrateException
Argument 1 passed to Drupal\Tests\migrate\Unit\TestMigrateExecutable::setSource() must be an instance of Drupal\Tests\migrate\Unit\MigrateSourceInterface, instance of Mock_SourcePluginBase_debde174 given, called in /opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php on line 297 and defined

/opt/sites/md82/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php:48
/opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php:297

6) Drupal\Tests\migrate\Unit\MigrateExecutableTest::testImportWithValidRowWithException
Argument 1 passed to Drupal\Tests\migrate\Unit\TestMigrateExecutable::setSource() must be an instance of Drupal\Tests\migrate\Unit\MigrateSourceInterface, instance of Mock_SourcePluginBase_debde174 given, called in /opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php on line 343 and defined

/opt/sites/md82/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php:48
/opt/sites/md82/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php:343

FAILURES!
Tests: 9, Assertions: 11, Errors: 6.
lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2621486-17-migrate-test-src-unit-d8.patch30.9 KB

Thank you @quietone for the tip in #16. This patch removes that type hint and adds back the changes to MigrateExecutableTest.php. Let's see what the test bot thinks now. My fingers are crossed!

benjy’s picture

benjy commented

Looks pretty good, just one question:

+++ b/core/modules/migrate/tests/src/Unit/SqlBaseTest.php
@@ -88,23 +92,43 @@ public function sqlBaseTestProvider() {
+      [FALSE, TRUE, TRUE,
+        ['username' => 'different_from_map', 'password' => 'different_from_map'],
...
+      [TRUE, TRUE, TRUE,
+        ['username' => 'same_value', 'password' => 'same_value'],

Should't we have one item per line? Is this valid to put 3 items on one line?

lars toomre’s picture

lars toomre commented
StatusFileSize
new interdiff-2621486-19-17.txt1.5 KB
new 2621486-19-migrate-tests-src-unit-d8.patch31.22 KB

Thanks for the review @benjy. I was unsure about the organization of the sub-arrays in that function. Attached are an interdiff and new patch breaking out each element onto its own line. Let me know if you see any other issues.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Great, thanks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2621486-19-migrate-tests-src-unit-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2621486-21-migrate-tests-src-unit-d8.patch30.39 KB

Here is a re-roll to get the patch to apply again.

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

Since there was a change I doubled checked and did a diff between the last two patches. I didn't see any issues. And, I agree with benjy, this is great. So, back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll 
git ac https://www.drupal.org/files/issues/2621486-21-migrate-tests-src-unit-d8.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 31124  100 31124    0     0  33194      0 --:--:-- --:--:-- --:--:-- 35651
error: patch failed: core/modules/migrate/tests/src/Unit/MigrationStorageTest.php:60
error: core/modules/migrate/tests/src/Unit/MigrationStorageTest.php: patch does not apply
lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2621486-25-migrate-tests-src-unit-d8.patch29.68 KB

Here is a re-roll of the patch from #21.

lars toomre’s picture

lars toomre commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC based on #23 and re-rolled patch in #25.

tvb’s picture

tvb commented
Issue tags: -Needs reroll

I could apply the patch, so no reroll is needed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 05f8dfc and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed bbd1baf on 8.1.x 
    Issue #2621486 by Lars Toomre: Fixes to migrate/tests/src/Unit/*.php...

Status: Fixed » Closed (fixed)

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