The unit tests of Shortcut's Migrate source plugins should be changed to the base class introduced in #2791119: Write meaningful Migrate source tests.

CommentFileSizeAuthor
#10 2807937-10.patch8.37 KBsvendecabooter
#7 2807937-3.patch8.27 KBerozqba
#5 2807937-2.patch8.27 KBerozqba
#2 2807937-1.patch8.31 KBerozqba

Comments

quietone created an issue. See original summary.

erozqba’s picture

erozqba commented
StatusFileSize
new 2807937-1.patch8.31 KB
erozqba’s picture

erozqba commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetTest.php
@@ -0,0 +1,41 @@
+  public function providerSource()
+  {

    The { needs to be on the same line as function providerSource(). 

  2. +++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetUsersTest.php
@@ -0,0 +1,41 @@
+  public function providerSource()
+  {

    Same here. 

  3. +++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutTest.php
@@ -0,0 +1,72 @@
+  public function providerSource()
+  {

    And here. 

  4. +++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutTest.php
@@ -0,0 +1,72 @@
+    // The expected results are identical to the source data.
+    $tests[0][1] = [
+      [
+        'mlid' => '473',
+        'menu_name' => 'shortcut-set-2',
+        'link_path' => 'admin/people',
+        'link_title' => 'People',
+        'weight' => '-50',
+      ],
+    ];

    This isn't identical to the source data...

erozqba’s picture

erozqba commented
Status: Needs work » Needs review
StatusFileSize
new 2807937-2.patch8.27 KB

Thanks a lot for the code review phenaproxima! Sorry for the issues, too many copy/paste. I'm uploading a new patch with your suggestions.

Status: Needs review » Needs work

The last submitted patch, 5: 2807937-2.patch, failed testing.

erozqba’s picture

erozqba commented
StatusFileSize
new 2807937-3.patch8.27 KB
erozqba’s picture

erozqba commented
Status: Needs work » Needs review
chipway’s picture

chipway commented
Status: Needs review » Needs work

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.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Needs work » Needs review
StatusFileSize
new 2807937-10.patch8.37 KB

Updated the patch to have more informative keys (next to descriptive comments) as suggested by alexpott

quietone’s picture

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

Thank you, svendecabooter.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed 
diff --git a/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetTest.php b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetTest.php
index 484a384..e1982de 100644
--- a/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetTest.php
+++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetTest.php
@@ -37,4 +37,5 @@ public function providerSource() {
 
     return $tests;
   }
+
 }
diff --git a/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetUsersTest.php b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetUsersTest.php
index f9c88dc..035aec3 100644
--- a/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetUsersTest.php
+++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutSetUsersTest.php
@@ -37,4 +37,5 @@ public function providerSource() {
 
     return $tests;
   }
+
 }
diff --git a/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutTest.php b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutTest.php
index 7077fa5..9f8fd3a 100644
--- a/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutTest.php
+++ b/core/modules/shortcut/tests/src/Kernel/Plugin/migrate/source/d7/ShortcutTest.php
@@ -68,4 +68,5 @@ public function providerSource() {
 
     return $tests;
   }
+
 }

Fixed on commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Committed and pushed c0fda56 to 8.3.x and 74312bb to 8.2.x. Thanks!

  • alexpott committed c0fda56 on 8.3.x 
    Issue #2807937 by erozqba, svendecabooter, phenaproxima: Convert...

  • alexpott committed 74312bb on 8.2.x 
    Issue #2807937 by erozqba, svendecabooter, phenaproxima: Convert...

Status: Fixed » Closed (fixed)

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