Follow up from #916388-154: Convert menu links into entities

shortcut_update_8000() should not use menu_link_save() and menu_load_links()

Files: 
CommentFileSizeAuthor
#6 1899682-interdiff-6.txt666 bytesandypost
#6 1899682-shortcut-upgrade-6.patch4.6 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,421 pass(es). View
#5 1899682-shortcut-upgrade-5.patch4.6 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#4 1899682-interdiff-4.txt2.93 KBandypost
#4 1899682-shortcut-upgrade-4.patch3.74 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#2 1899682-shortcut-upgrade-2.patch3.84 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,313 pass(es). View
#1 1899682-update-1.patch821 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 49,298 pass(es). View

Comments

andypost’s picture

Status: Active » Needs review
FileSize
821 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,298 pass(es). View
andypost’s picture

Title: Fix upgrade path for shortcut module » Add upgrade path tests for shortcut module
Issue tags: +Configuration system, +Configurables
FileSize
3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 49,313 pass(es). View

Added full test coverage and proper upgrade path

dawehner’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutUpgradePathTest.phpundefined
@@ -0,0 +1,66 @@
+  public static function getInfo() {

Missing empty line at the beginning and the end of the class

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutUpgradePathTest.phpundefined
@@ -0,0 +1,66 @@
+    db_insert('shortcut_set')->fields(array(
+      'set_name',
+      'title',
+    ))
+      ->values(array(
+        'set_name' => 'shortcut-set-2',
+        'title' => 'Custom shortcut set',
+      ))

Shouldn't we put this test data into a .database.php file? I'm not sure, but at least make a new line after db_insert(), this would improve the readability.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutUpgradePathTest.phpundefined
@@ -0,0 +1,66 @@
+    $this->assertTrue(isset($set->uuid), 'Converted set has a UUID');
...
+    $this->assertTrue(isset($set->uuid), 'Converted set has a UUID');

What about using $set->uuid() ?

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutUpgradePathTest.phpundefined
@@ -0,0 +1,66 @@
+    // Assert that manifest has been created and contains the expected records.

I'm wondering whether a more indirect test should be done, so removing the manifest files would be easier, but yeah let's keep in there for now.

andypost’s picture

FileSize
3.74 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
2.93 KB

@dawehner thanx for review!

Here's a fixed version. Also moved test file to proper place

I've checked the existing tests for manifests again and they are the same in FieldUpgradePathTest and ContactUpgradePathTest

andypost’s picture

FileSize
4.6 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Messed the database file

andypost’s picture

FileSize
4.6 KB
PASSED: [[SimpleTest]]: [MySQL] 49,421 pass(es). View
666 bytes

Finally last copy/paste error

andypost’s picture

Issue tags: +D8 upgrade path

taggin

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Hopefully, testbot will agree.

TBH, I could live without the additional upgrade path test. It performs yet another upgrade path of a full-blown Standard installation, which includes like... a gazillion of modules + updates.

By now, we're probably duplicating the basic Standard profile upgrade path test in a dozen of tests already. All of them are slowing down the total test suite time. :(

Thus, I wouldn't mind if that new test would be left out of the commit.

andypost’s picture

@sun I filed #1899858: Consider a proper place for upgrade test
Once we move all upgrade tests in one folder we can simply merge them in one tremendous test so minify the count of installs

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed (including the test, can't have too many upgrade path tests), to 8.x.

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