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
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-23.txt | 665 bytes | xjm |
#23 | 2614824-23.patch | 2.96 KB | xjm |
#18 | interdiff-18.txt | 2.15 KB | xjm |
#18 | 2614824-18.patch | 2.97 KB | xjm |
#17 | 14-17-interdiff.txt | 2 KB | alexpott |
Comments
Comment #4
catchComment #5
xjmDifferent 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.
Comment #6
penyaskito@xjm: Probably you already know that, but for the record #2615776: LocalePluralFormatTest is failing on Postgres fixed those.
Comment #7
xjmAt least it's reliable:
https://www.drupal.org/pift-ci-job/85956
Comment #8
alexpottSo 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.
Comment #10
alexpottComment #11
alexpottComment #12
xjmLet's add inline comments explaining the use of
t()
in these assertions.@alexpott says this is some separate bug that he is looking for.
Let's add inline comments specifically explaining why these assertions do NOT use
t()
.Comment #13
alexpottSo 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.
Comment #14
alexpott@xjm thanks for the review.
Re #12
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 oft()
. However I've documented the issue on the class docblock.Comment #15
alexpottI've created #2697291: Config translation form labels are not translated for the label issue.
Comment #16
xjm@alexpott and I discussed point 3 a bit more as well as some other docs improvements.
Comment #17
alexpottHere's a patch with @xjm's suggestions
Comment #18
xjmTiny 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 usingt()
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.
Comment #19
xjmComment #20
tstoecklerPlease update the issue summary so people can understand what is going on.
Comment #21
xjmIf an issue has no summary and only core committers hear it, does it make a sound? ;)
Added a brief explanation.
Comment #22
catchVery nit, but:
semicolon instead of and?
Comment #23
xjmAgreed; that makes it less of a run-on.
Re-RTBCing since it's a grammar nit.
Comment #24
xjmAha, I knew I had just seen something about this! #2600798: Translation files import not working properly when installing dev version describes that bug.
Comment #25
tstoecklerThanks for the explanation, that makes a lot of sense!
Comment #29
catchCommitted/pushed to all three 8.x branches, thanks!