Problem/Motivation

See branch test history at https://www.drupal.org/pift-ci-job/79412

In NodeTypeTranslationTest, the test child site is installed in French and, therefore, assertions on translated strings check against the French translation.

When Drupal is on a stable version constant (e.g. 8.0.5 rather than 8.0.x-dev), the non-interactive installer downloads and installs the French translation from l.d.o. This does not happen with dev versions, so most of the time, it makes no difference (since the French "translation" is actually just the English string) and the test passes. However, there are a few incorrect uses of t() in the test that cause it to fail when the translation is actually installed. (One assertion in the test is actually not testing what it is supposed to because of these bugs.)

Proposed resolution

Fix the test.

Remaining tasks

Followups to fix the facts that: 1. Non-interactive installation on a dev version doesn't get the translation 2. There are larger problems with the test being affected by something external and unpredictable.

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, rc4.patch, failed testing.

The last submitted patch, rc4.patch, failed testing.

catch’s picture

Priority: Normal » Major
xjm’s picture

Different fails related to translations:
https://www.drupal.org/pift-ci-job/80754
https://www.drupal.org/pift-ci-job/80726

Edit: the second one might be postgres-specific.

penyaskito’s picture

@xjm: Probably you already know that, but for the record #2615776: LocalePluralFormatTest is failing on Postgres fixed those.

xjm’s picture

alexpott’s picture

So this is because when the version constant is not dev when we install in French we get French translations. On -dev it does not download a translation this is by design.

The last submitted patch, 8: 2614824-8.test-only.patch, failed testing.

alexpott’s picture

Version: 8.0.x-dev » 8.2.x-dev
alexpott’s picture

Version: 8.2.x-dev » 8.0.x-dev
xjm’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/node/src/Tests/NodeTypeTranslationTest.php
    @@ -105,12 +105,12 @@ public function testNodeTypeTranslation() {
    -    $this->assertRaw(t('Create @name', array('@name' => $translated_name)));
    +    $this->assertRaw(t('Create @name', array('@name' => $translated_name), array('langcode' => $langcode)));
    ...
    -    $this->assertRaw(t('Create @name', array('@name' => $translated_name)));
    +    $this->assertRaw(t('Create @name', array('@name' => $translated_name), array('langcode' => $langcode)));
    

    Let's add inline comments explaining the use of t() in these assertions.

  2. +++ b/core/modules/node/src/Tests/NodeTypeTranslationTest.php
    @@ -128,17 +128,16 @@ public function testNodeTypeTitleLabelTranslation() {
    -    $this->assertRaw(t('Label'));
    

    @alexpott says this is some separate bug that he is looking for.

  3. +++ b/core/modules/node/src/Tests/NodeTypeTranslationTest.php
    @@ -128,17 +128,16 @@ public function testNodeTypeTitleLabelTranslation() {
    -    $this->assertRaw(t('Edited title'));
    +    $this->assertText('Edited title');
    ...
    -    $this->assertRaw(t('Edited title'));
    +    $this->assertText('Edited title');
    ...
    -    $this->assertRaw(t('Translated title'));
    +    $this->assertText('Translated title');
    

    Let's add inline comments specifically explaining why these assertions do NOT use t().

alexpott’s picture

+++ b/core/modules/node/src/Tests/NodeTypeTranslationTest.php
@@ -128,17 +128,16 @@ public function testNodeTypeTitleLabelTranslation() {
-    $this->assertRaw(t('Label'));

So this is debatable whether this is a bug the labels on the config translation page are in english. Here is a screenshot of translating a content type to Spanish on a site installed in French.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
2.66 KB

@xjm thanks for the review.
Re #12

  1. Added comments
  2. See #13
  3. I didn't add comments here because the use of t() was completely wrong as this text is hardcoded in the test - it does not fail if the version is 8.2.0 but it just seems sane to look at all the assertions in the test for use of t(). However I've documented the issue on the class docblock.
alexpott’s picture

xjm’s picture

@alexpott and I discussed point 3 a bit more as well as some other docs improvements.

alexpott’s picture

Here's a patch with @xjm's suggestions

xjm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.97 KB
2.15 KB

Tiny tidbits of grammar polish in the attached.

There are a few larger issues this touches on: @alexpott and I discussed that, in general, tests should not be affected by external translations. They should either only test against the English strings that ship with core, or when translation itself is being tested, they should possibly test against a fixture instead of an automatically downloaded translation. @alexpott referenced #2031175: Translation imports take a lot of time in tests even when not needed. This also points to our overuse of t() in tests in general. For background, in #500866: [META] remove t() from assert message, we decided to stop using t() on assertion messages, but to keep using it for UI strings the assertion was checking. We should reconsider that (could use its own issue).

Those things are all much bigger discussions, though, and this fixes a bug that is especially vexing for release managers. So RTBC.

xjm’s picture

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Please update the issue summary so people can understand what is going on.

xjm’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

If an issue has no summary and only core committers hear it, does it make a sound? ;)

Added a brief explanation.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Very nit, but:

+++ b/core/modules/node/src/Tests/NodeTypeTranslationTest.php
@@ -14,6 +14,10 @@
+ * Note that the child site is installed in French and, therefore, when making

semicolon instead of and?

xjm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.96 KB
665 bytes

Agreed; that makes it less of a run-on.

Re-RTBCing since it's a grammar nit.

xjm’s picture

tstoeckler’s picture

Thanks for the explanation, that makes a lot of sense!

  • catch committed 463d844 on 8.2.x
    Issue #2614824 by alexpott, xjm, catch: Tests fail when version constant...

  • catch committed ff551ae on 8.1.x
    Issue #2614824 by alexpott, xjm, catch: Tests fail when version constant...

  • catch committed 6790311 on 8.0.x
    Issue #2614824 by alexpott, xjm, catch: Tests fail when version constant...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches, thanks!

Status: Fixed » Closed (fixed)

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