Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
821 bytes
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

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

@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

Messed the database file

andypost’s picture

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.