While working on #2030537: Translations not downloaded when adding a new language I found a few things in Interface Translation (locale) module's test that could be cleaned-up.

  • Not used 'use' statements.
  • Definition of modules to be enabled (public static $modules) that were already defined by the parent class (LocaleUpdateBase).
  • A faulty "Contains" comment.
CommentFileSizeAuthor
#8 interdiff.txt726 bytesbenjf
#7 2338011-locale-test-cleanup-7.patch3.55 KBbenjf
locale-test-cleanup.patch2.57 KBSutharsan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Cleanup of locale tests. » Cleanup of locale tests
Issue tags: +D8MI, +language-ui, +sprint 
+++ b/core/modules/locale/src/Tests/LocaleUpdateInterfaceTest.php
@@ -21,7 +19,7 @@ class LocaleUpdateInterfaceTest extends LocaleUpdateBase {
-  public static $modules = array('update', 'locale', 'locale_test_translate');
+  public static $modules = array('locale_test_translate');

But locale_test_translate does not depend on locale and/or update?

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

LocaleUpdateInterfaceTest extends LocaleUpdateBase (test base class). LocaleUpdateBase class defines: public static $modules = array('update', 'update_test', 'locale', 'locale_test');. The locale_test_translate module does not depend on any module. LocaleUpdateInterfaceTest requires 'locale', 'locale_test' and 'locale_test_translate'. I have looked at a few test classes that extend a base test class. (most) test classes that extend test base classes do not re-define the required modules that are already defined by the base test class. Does that answer your question?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

But its the same public static property. How redefining it will not make those base modules removed?

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

Dunno, it works (not my most convincing answer ;) ). Perhaps better: a few examples that do the same:

  • FileNormalizeTest extends NormalizerTestBase
  • EmailFieldRdfaTest extends FieldRdfaTestBase
  • FieldUITest extends FieldTestBase
  • CommentFieldFilterTest extends CommentTestBase
  • BlockSystemBrandingTest extends BlockTestBase
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I don't see how that one would work well given that in PHP the extending class overrides the base class' property when doing this. The other two changes look perfect BTW :)

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

It is because of $modules is a static property. This is the issue that made it happen: #1380958: Replace $modules list for WebTestBase::setUp() with ::$modules class properties. WebTestBase cycles through (parent) classes and collects all modules.

benjf’s picture

benjf CreditAttribution: benjf commented
FileSize
2338011-locale-test-cleanup-7.patch3.55 KB

Updated patch with a few more unused 'use' statements removed. Otherwise, it looks good to me!

benjf’s picture

benjf CreditAttribution: benjf commented
FileSize
interdiff.txt726 bytes

adding interdiff for the last patch

YesCT’s picture

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

I read the whole patch.
Changes all look good.
Inspected locale tests and this gets all the un-used uses there.

We can keep this small and not expand the scope too far. A good small improvement.

alexpott’s picture

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

Committed ab50db3 and pushed to 8.0.x. Thanks!

  • alexpott committed ab50db3 on 8.0.x 
    Issue #2338011 by benjf, Sutharsan: Cleanup of locale tests.
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Yay, thanks. Also thanks @Sutharsan for diving in and helping me learn :)

Status: Fixed » Closed (fixed)

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