Problem/Motivation

The Migrate UI was introduced to with only basic tests added in #2647470: Write tests. We should test each migration to at least show that it is working. At the moment the tests are green even though some of the migration failed.

Proposed resolution

Add more tests and work why some of the things are not migrating.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
1.84 KB

This will fail.

alexpott’s picture

This fixes the telephone field migration... I'm not sure that failed is the correct status if the telephone module is not installed on the destination... but that is not the point of this patch.

The last submitted patch, 2: 2683579-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2683579-3.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
50.01 KB
52.27 KB

So this patch should be green - and now we have proper testing of setting the source_base_path through the UI!

Status: Needs review » Needs work

The last submitted patch, 6: 2683579-6.patch, failed testing.

alexpott’s picture

Hmmm weird... that test passes locally for me...

alexpott’s picture

Status: Needs work » Needs review
FileSize
188 bytes
52.63 KB

Ah my gitignore causing me an issue... missing the temp file...

benjy’s picture

FileSize
55.1 KB
1.01 KB

Added the fix to run these tests locally because it gets me every time, and added the missing temp file.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

I only made one change to the test while debugging so I think I can still RTBC this, looks good.

alexpott’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
53.3 KB

Rerolled on top of the plugins patch - until that is in 8.1.x this is only eligible for 8.2.x

The only thing that had to change was how we loaded the migrations to see if they were successful.

commit 080d305537fac91f8a055bb1fbbb542023ebf473
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Wed Mar 9 14:04:37 2016 +0000

    Fix migration loading

diff --git a/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
index 573f687..af91b4d 100644
--- a/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
+++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
@@ -8,14 +8,15 @@
 namespace Drupal\migrate_drupal_ui\Tests;
 
 use Drupal\Core\Database\Database;
-use Drupal\migrate\Entity\Migration;
 use Drupal\migrate\Plugin\MigrateIdMapInterface;
+use Drupal\migrate_drupal\MigrationCreationTrait;
 use Drupal\simpletest\WebTestBase;
 
 /**
  * Provides a base class for testing migration upgrades in the UI.
  */
 abstract class MigrateUpgradeTestBase extends WebTestBase {
+  use MigrationCreationTrait;
 
   /**
    * Use the Standard profile to test help implementations of many core modules.
@@ -153,8 +154,11 @@ protected function testMigrateUpgrade() {
       $this->assertEqual($expected_count, $real_count, "Found $real_count $entity_type entities, expected $expected_count.");
     }
 
-    /* @var \Drupal\migrate\Entity\Migration $migration */
-    foreach (Migration::loadMultiple() as $migration) {
+    $version_tag = 'Drupal ' . $this->getLegacyDrupalVersion($this->sourceDatabase);
+    $plugin_manager = \Drupal::service('plugin.manager.migration');
+    /** @var \Drupal\migrate\Plugin\Migration[] $all_migrations */
+    $all_migrations = $plugin_manager->createInstancesByTag($version_tag);
+    foreach ($all_migrations as $migration) {
       $id_map = $migration->getIdMap();
       foreach ($id_map as $source_id => $map) {
         $source_id_values = array_values(unserialize($source_id));

Basically we can use the MigrationCreationTrait to do this for us.

Status: Needs review » Needs work

The last submitted patch, 12: 2683579-12.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#12 got tested against 8.1.x and not 8.2.x... testing now against 8.2.x

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

xjm’s picture

Issue tags: -8.1.0 release notes
catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
@@ -150,6 +153,29 @@ protected function testMigrateUpgrade() {
+        $source_id_values = array_values(unserialize($source_id));

Why array_values()? Could use a comment if needed.

Also can't see what the added images are used for - there's no obvious test changes that indicate whether they'd pass or fail due to the presence of the images or not, or I'm missing something. Test-only patch might be enough to answer that.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
884 bytes
53.45 KB

For reasons I don't quite understand the id map is really tricky to work with :( - added a comment. But this is not the fault of this issue.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Still missing what difference the images make.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.26 KB
53.45 KB

The last submitted patch, 21: 2683579-21.test-only.patch, failed testing.

  • catch committed 8b3a700 on 8.2.x
    Issue #2683579 by alexpott, benjy: Improve migrate UI tests
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

xjm’s picture

Version: 8.2.x-dev » 8.1.x-dev

Status: Fixed » Closed (fixed)

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