Problem/Motivation

Follow-up from #3389668: Split up ConfigTranslationUiTest - we have a whole test class for date formats with one method, and a huge translation ui test that also has a date formats test. The date format tests could live together in one class.

This requires two very minor changes to the test method that gets moved:

1. Don't try to log in - it already happens in the constructor
2. The big ConfigTranslationUi test uses French, the date formats test uses Spanish and German, switched to testing against German, only about three lines change.

I think this will help test performance marginally because the huge test class is still one of the slowest at well over 6 minutes but it also seems like a better place for it to live.

Also there are two site information tests methods, if we move those out to their own class, I think that will save potentially over a minute.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3390692

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Active » Needs review

catch opened merge request !4906

catch’s picture

catch
he/him
Primary language English
commented
Title: Move config ui date format test to the dedicated test class » Split out a couple more config translation ui tests
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For cc issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Fixed the unused use statements.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks taking a look at the moves and agree with the placement of the functions. Don't think any test coverage was lost either.

lauriii made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented

A cspell:ignore needed to move too, probably dictionary changes elsewhere uncovered it.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Split out a couple more config translation ui tests » Split out a couple more config translation UI tests

Saving credits.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

See on the MR regarding French vs. German.

I confirmed that this change set is purely moved test code aside from the following changes to that method:

diff --git a/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php b/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php
index 579466f730..40b6e5bec6 100644
--- a/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php
+++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php
@@ -5,7 +5,7 @@
 use Drupal\Core\Language\Language;
 use Drupal\Core\Language\LanguageInterface;
 
-// cspell:ignore libellé viewsviewfiles
+// cspell:ignore viewsviewfiles
 
 /**
  * Translate settings and entities to various languages.
@@ -124,7 +124,6 @@ public function testSiteInformationTranslationUi() {
    * Tests date format translation.
    */
   public function testDateFormatTranslation() {
-    $this->drupalLogin($this->adminUser);
 
     $this->drupalGet('admin/config/regional/date-time');
 
@@ -151,7 +150,7 @@ public function testDateFormatTranslation() {
       $this->drupalGet($translation_base_url);
 
       // 'Add' link should be present for French translation.
-      $translation_page_url = "$translation_base_url/fr/add";
+      $translation_page_url = "$translation_base_url/de/add";
       $this->assertSession()->linkByHrefExists($translation_page_url);
 
       // Make sure original text is present on this page.
@@ -163,7 +162,7 @@ public function testDateFormatTranslation() {
 
       // Update translatable fields.
       $edit = [
-        'translation[config_names][core.date_format.' . $id . '][label]' => $id . ' - FR',
+        'translation[config_names][core.date_format.' . $id . '][label]' => $id . ' - DE',
         'translation[config_names][core.date_format.' . $id . '][pattern]' => 'D',
       ];
 
@@ -172,16 +171,16 @@ public function testDateFormatTranslation() {
       $this->submitForm($edit, 'Save translation');
 
       // Get translation and check we've got the right value.
-      $override = \Drupal::languageManager()->getLanguageConfigOverride('fr', 'core.date_format.' . $id);
+      $override = \Drupal::languageManager()->getLanguageConfigOverride('de', 'core.date_format.' . $id);
       $expected = [
-        'label' => $id . ' - FR',
+        'label' => $id . ' - DE',
         'pattern' => 'D',
       ];
       $this->assertEquals($expected, $override->get());
 
       // Formatting the date 8 / 27 / 1985 @ 13:37 EST with pattern D should
       // display "Tue".
-      $formatted_date = $this->container->get('date.formatter')->format(494015820, $id, NULL, 'America/New_York', 'fr');
+      $formatted_date = $this->container->get('date.formatter')->format(494015820, $id, NULL, 'America/New_York', 'de');
       $this->assertEquals('Tue', $formatted_date, 'Got the right formatted date using the date format translation pattern.');
     }
   }
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Replied on the MR, and pushed a commit changing the comment - it's to make the method compatible with the test class it's being moved to, which only uses German and Spanish at the moment.

  • xjm committed d170d77f on 11.x 
    Issue #3390692 by catch, xjm, smustgrave: Split out a couple more config...

  • xjm committed 42968336 on 10.2.x 
    Issue #3390692 by catch, xjm, smustgrave: Split out a couple more config...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Reviewed the whole method and looked at the parent test as well. Looks good now.

Committed to 11.x and 10.2.x. Thanks! As a test performance improvement, this is also a good choice for backport to 10.1.x, but it did not cherry-pick cleanly. Setting PTBP for a 10.1.x backport.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.1.x-dev » 10.2.x-dev
Status: Patch (to be ported) » Fixed

Final patch release of 10.1 is out, so this can go back to "Fixed".

Status: Fixed » Closed (fixed)

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