Problem/Motivation

The list of modules in MigrateUpgradeTestBase can't be over ridden which is inconvenient/annoying when extending this class in contrib. Tests extending this class will have modules enabled that they may not need or want.

The modules listed are those needed for both the d6 and d7 tests, and it is somewhat convenient to have a shared list in one place. However, the modules to install is dependent on what is enabled in the source database and really should be in the respective d6 and d7 tests.

/**
* Modules to enable.
*
* @var array
*/
public static $modules = [
'language',
'content_translation',
'migrate_drupal_ui',
'telephone',
'aggregator',
'book',
'forum',
'statistics',
'migration_provider_test',
];

Proposed resolution

Remove $modules from MigrateUpgradeTestBase
Move the module list to the d6 and d7 tests.

Remaining tasks

Review

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#2 2916199-2.patch2.38 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 2916199-2.patch2.38 KB
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Assigned: Unassigned » heddn

Assigning to myself to review this week.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Assigned: heddn » Unassigned
Status: Needs review » Reviewed & tested by the community

We've also got Drupal\migrate_drupal_ui\Tests\MigrateUpgradeTestBase. Should it also have the modules moved into the d6/d7 implementation classes? Ah, no. Because it is deprecated. So, looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2916199-2.patch, failed testing. View results

quietone’s picture

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

Tests are passing, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2916199-2.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Reviewed & tested by the community

Revert status after random fail.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
@@ -15,9 +15,22 @@
-  public static $modules = ['file'];
+  public static $modules = [
+    'file',
+    'language',
+    'content_translation',
+    'migrate_drupal_ui',
+    'telephone',
+    'aggregator',
+    'book',
+    'forum',
+    'statistics',
+    'migration_provider_test',
+  ];

Hm, this was explicitly only file module before. Why are we enabling all the things now?

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Reviewed & tested by the community

re #9: it wasn't just file. It was file, plus all the things in MigrateUpgradeTest. Which is the entire point for this issue. In commerce migrate, we don't want to enable all the things when we extend from MigrateUpgradeTest. We prefer to see that pushed down into the concrete classes. Yes that means more duplication, but it gives the abstract class more utility.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

So that is because test cases special case this property, see FunctionalTestSetupTrait::installModulesFromClassProperty(). Thanks @phenaproxima for the digging. Thanks!

  • Gábor Hojtsy committed 1d22895 on 8.5.x 
    Issue #2916199 by quietone, heddn: MigrateUpgradeTestBase shouldn't...
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue tags: +Migrate January 2017 Sprint
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue tags: -Migrate January 2017 Sprint +Migrate January 2018 Sprint

Status: Fixed » Closed (fixed)

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