Problem/Motivation

To keep existing test coverage and to reduce the size of the patch in #2746541: Migrate D6 and D7 node revision translations to D8. Plus, this work can be considered a step of #2974445: [Meta] Refactor Migrate Drupal UI Functional tests.

So, what are we doing here? In the node revision translation issue changes are needed to the functional test of a site migration via the UI, which effects the output displayed on the IdConflict page. The node revision translation issue must start from an empty D8 database which it isn't at the moment, but it used to test the IdConflict page. So, lets keep those changes and make a new test for just the IdConflict form so we don't lose test coverage.

When this is committed the test of the idConflict page with all the preloaded content, added by CreateTestContentEntitiesTrait::createContent() will be done twice, once in the new tests and once in the full migration test. That will change to just the tests in this issue when #2746541: Migrate D6 and D7 node revision translations to D8 is committed.

Proposed resolution

Add separate tests of the IdConflict form for d6 an d7 by starting with the ones in patch #229
Pull out the IdConflict form test, with preloaded content to its own test for D6 and D7. Keep the changes to a minimum here because there is an issue to refactor all migrate_drupal_ui functional tests, #2974445: [Meta] Refactor Migrate Drupal UI Functional tests, which they very much need. While that issue is still at NR but is being worked on steadily.

Remaining tasks

Review and commit

CommentFileSizeAuthor
#14 3087061-14.patch7.57 KBquietone
#14 interdiff-10-14.txt2.21 KBquietone
#10 3087061-10.patch8.07 KBquietone
#10 interdiff-7-10.txt711 bytesquietone
#7 3087061-7.patch8.08 KBquietone
#7 interdiff-2-7.txt1.87 KBquietone
#2 3087061-2.patch8.31 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 3087061-2.patch8.31 KB

Just copying the files over.

quietone’s picture

quietone commented
Issue summary: View changes

This is ready for review.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Issue tags: +Migrate critical

Tagging migrate critical because it is needed for #2746541: Migrate D6 and D7 node revision translations to D8.

quietone’s picture

quietone commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -0,0 +1,141 @@
+ * The test method is provided by the MigrateUpgradeTestBase class.

    No it is not. remove 

  2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -0,0 +1,141 @@
+   * Modules to enable.

    Use inheritdoc 

  3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -0,0 +1,141 @@
+   * Executes all steps of migrations upgrade.

    Tests Id conflict form

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2-7.txt1.87 KB
new 3087061-7.patch8.08 KB

Corrected comments as above. Also removed @legacy as there is no legacy code being tested here.

quietone’s picture

quietone commented
Issue summary: View changes
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented 
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/IdConflictTest.php
@@ -0,0 +1,138 @@
+ * Tests Drupal 7 Id Conflict page.
...
+    $this->loadFixture(drupal_get_path('module', 'migrate_drupal') . '/tests/fixtures/drupal6.php');

Shouldn't the Drupal 7 test be using the Drupal 7 fixture?

quietone’s picture

quietone commented
StatusFileSize
new interdiff-7-10.txt711 bytes
new 3087061-10.patch8.07 KB

Yes, it should. Should be fixed here.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Reviewed & tested by the community

Makes sense, this seems to be a good first step. This patch does nothing but add test coverage, so lets get it committed quickly.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
  1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -0,0 +1,136 @@
+  protected $nodeStorage;

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/IdConflictTest.php
@@ -0,0 +1,138 @@
+  protected $nodeStorage;

    this don't appear to be used? 

  2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -0,0 +1,136 @@
+    if ($version == 6) {

    if this is a d6 test - do we need to check this?

  3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/IdConflictTest.php
@@ -0,0 +1,138 @@
+    if ($version == 6) {

    if this is a d7 test - why do we have this? are we duplicating code here on purpose? should we be putting the common stuff in a trait and using the trait in both these classes?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review
quietone’s picture

quietone commented
StatusFileSize
new interdiff-10-14.txt2.21 KB
new 3087061-14.patch7.57 KB

Fixes for the items in #13.

we duplicating code here on purpose?

Yes, for two reasons. First, this test needed to be split into a d6 an d7 version in the node revision translation issue. Doing that just increased the size of that already big and complicated patch, which I think it makes it harder to focus on the node migration changes. So, moving this part to a separate issue will help 'declutter', for lack of a better word, the node revision translation patch. The second reason is that there is an issue to refactor all the migrate_drupal_ui functional tests. (Which reminds me I need to upload my last version of that refactoring).

I haven't run the tests locally.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Reviewed & tested by the community

feedback addressed. back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

As this is additional test coverage backported all the way to 8.8.x

Committed and pushed 83e62065d0 to 9.0.x and af807602d2 to 8.9.x and 27598bdb40 to 8.8.x. Thanks!

  • alexpott committed 83e6206 on 9.0.x 
    Issue #3087061 by quietone, larowlan, Lendude: Move existing assertions...

  • alexpott committed af80760 on 8.9.x 
    Issue #3087061 by quietone, larowlan, Lendude: Move existing assertions...

  • alexpott committed 27598bd on 8.8.x 
    Issue #3087061 by quietone, larowlan, Lendude: Move existing assertions...

Status: Fixed » Closed (fixed)

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