Problem/Motivation

A follow up to #2953360: Experimental migrate_drupal_multilingual module.

The names of the full upgrades tests are do not reflect what the test does. Tests such as MigrateUpgrade6Test were test the full upgrade when multilingual modules are enabled and tests such as MigrateUpgrade6I18ReviewPageTest test the review page when the multilingual modules are not enabled.. Pretty obvious that is not what you expect when reading the name.

This is just historical, as each migration was written it was added to MigrateUpgrade6Test or MigrateUpgrade7Test, and that included any migration for translations. It wasn't until MigrateUpgrade6I18nReviewPageTest was added, in #2960013: i18n menu links translation in wrong directory, that a test was added that excluded the translations but the renaming didn't happen then.

Proposed resolution

Rename as follows

    MigrateUpgradeReviewPageTestBase.php -> MultilingualReviewPageTestBase.php
    MigrateUpgradeI18nReviewPageTestBase.php -> NoMultilingualReviewPageTestBase.php
    MigrateUpgrade6ReviewPageTest.php -> MultilingualReviewPageTest.php
    MigrateUpgrade6I18nReviewPageTest.php -> NoMultilingualReviewPageTest.php
    MigrateUpgrade6NoMultilingualTest.php -> NoMultilingualTest.php
    MigrateUpgrade6Test.php -> Upgrade6Test.php
    MigrateUpgrade7ReviewPageTest.php -> MultilingualReviewPageTest.php
    MigrateUpgrade7NoMultilingualTest.php -> NoMultilingualTest.php
    MigrateUpgrade7Test.php ->Upgrade7Test.php
CommentFileSizeAuthor
#10 Screenshot from 2019-01-16 06-30-01.png38.34 KBquietone
#9 Screen Shot 2019-01-14 at 10.39.44 PM.png99.72 KBKristen Pol
#9 Screen Shot 2019-01-14 at 10.41.17 PM.png76.93 KBKristen Pol
#8 interdiff-2-8.txt2.16 KBquietone
#8 2987418-8.patch9.03 KBquietone
#2 2987418-2.patch9.02 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2987418-2.patch9.02 KB

Updated the IS to reflect the names used.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

This is ready for review. When this is committed many migrate issues will need rerolling but the sooner this is done the better. The names have always been confusing and I made them (Sigh) and use them a lot.

When reviewing please check that the docbloc for the test class makes sense to you and suggest changes, if it does not.

anpolimus’s picture

anpolimus
Primary language Ukrainian
Location Kyiv
CreditAttribution: anpolimus as a volunteer and at DOOR3, Drupal Ukraine Community commented

Test rename sounds reasonable.
This code could break something, but since this is an initial point of reroll, code is good.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@anpolimus, thanks.

This will only break tests in contrib that extend these classes and there won't be many of them. I tried searching http://grep.xnddx.ru/ for instances where these classes are used in contrib and it doesn't find any occurances, which is incorrect.

I am not sure what is mean by "the initial point of a reroll". This is not a reroll and the patch is not complete, unless it is set to NW for something.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Thanks for the patch. I noticed some minor wording things that could be addressed.

  1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MultilingualReviewPageTestBase.php
@@ -6,8 +6,10 @@
  * Provides a base class for testing the review step of the Upgrade form.
+ *
+ * When using this test class enable translation modules.

    Nitpick: Suggest comma after "class" so it is easier to understand meaning, e.g.

    When using this test class, enable the translation modules.

  2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/NoMultilingualReviewPageTestBase.php
@@ -5,9 +5,9 @@
+ * When using this test class do not enable migrate_drupal_multilingual.

    Nitpick: Suggest comma after "class" so it is easier to understand meaning, e.g.

    When using this test class, do not enable the migrate_drupal_multilingual module.

  3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualReviewPageTest.php
@@ -2,15 +2,17 @@
+ * Tests without translation modules and migrate_drupal_multilingual enabled.

    Nitpick: Wording seems a bit unclear to me. Perhaps something like:

    Tests with the translation modules and migrate_drupal_multilingual module disabled.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

How can we test this?

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
2987418-8.patch9.03 KB
interdiff-2-8.txt2.16 KB

@Kristen Pol, yes what you suggest is easier to understand. This patch fixes all the items in #6.

As for how can we test this? I would think that just checking that the tests are running is sufficient, which I did. I reviewed the full log of the test out and confirmed that all 7 of the tests are still running. The test base classes are only used by migrate_drupal_ui.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
FileSize
Screen Shot 2019-01-14 at 10.41.17 PM.png76.93 KB
Screen Shot 2019-01-14 at 10.39.44 PM.png99.72 KB

I reviewed the interdiff and the changes look fine. I looked at the tests that were fired here:

https://dispatcher.drupalci.org/job/drupal_patches/81022/consoleText

and see passes for Upgrade6Test and Upgrade7Test and I see the classes from the patch in the log as well.

You wrote:

" I reviewed the full log of the test out and confirmed that all 7 of the tests are still running. The test base classes are only used by migrate_drupal_ui."

So, I'm not sure my review of the test logs is sufficient.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
Screenshot from 2019-01-16 06-30-01.png38.34 KB

Ah, yes, the log output is truncated per line which is not helpful. Sorry, I could have added more detail on what I did. If you search for \Drupal\Tests\migrate_drupal_ui\d' and the other 5 tests are found.

This screenshot is made using the link you posted for the console output

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Reviewed & tested by the community

Oh! Sorry I didn't catch that. Given that info from #10, marking RTBC.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@Kristen Pol, thanks for making sure this is working correctly!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2987418-8.patch, failed testing. View results

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Version: 8.6.x-dev » 8.7.x-dev

moving to 8.7.x

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs work » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 898083ce7b to 8.7.x and 1ad9e26905 to 8.6.x. Thanks!

  • catch committed 898083c on 8.7.x 
    Issue #2987418 by quietone, Kristen Pol: Rename MigrateUpgrade tests

  • catch committed 1ad9e26 on 8.6.x 
    Issue #2987418 by quietone, Kristen Pol: Rename MigrateUpgrade tests

(...

Status: Fixed » Closed (fixed)

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