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

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
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 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
commented

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

sutharsan’s picture

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
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 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 commented
StatusFileSize
new 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 commented
StatusFileSize
new interdiff.txt726 bytes

adding interdiff for the last patch

yesct’s picture

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 🇪🇺🌍
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
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.