There is no need to use t() in tests, unless we're testing translations, however in core we do not follow this consistently, which does not set a good example for new contributions.
In #3133726: [meta] Remove usage of t() in tests not testing translation we identified there are several calls to t() in calls to clickLink() and that removing all these in one go seems to be a suitable way of attacking this problem.
Identify and remove all calls to t() wrapped in calls to clickLink(), except those used by translation-related code (if any).
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 3153469-27-9.2.x.patch | 90.49 KB | longwave |
| #22 | 3153469-22-without-deprecation.patch | 91.07 KB | hmendes |
Comments
Comment #2
hardik_patel_12 commentedKindly review the patch.
Comment #3
joseph.olstadIf I install Drupal in another language using this patch and run the tests, they will fail.
Not sure about the rationale behind this change?
Comment #5
hardik_patel_12 commented@joseph.olstad , There is no need to use t() in tests, unless we're testing translations, however in core we do not follow this consistently, which does not set a good example for new contributions. We will need to ensure to leave t() where the test actually tests the translation system.
Solving failed test cases.
Comment #7
joseph.olstadwhat if we want to test an install profile for automated tests and that install profile uses a default language other than english?
those tests would break, no?
For such a minuscule performance difference, not sure if this change is worth the effort given that at some point it might be useful to test an install profile test runner in a language other than english.
Comment #9
longwaveI'm going to rescope this a bit tighter as the three methods in the original issue don't seem very closely related, and at the time of writing
clickLink()is one of the biggest remaining uses oft()calls.Comment #10
longwaveComment #11
longwaveLet's add a deprecation so we can find the remaining uses.
Comment #13
longwaveFixed the remaining failures.
clickLinkalready asserts that the link exists, so we can remove some redundancy here too.Comment #14
longwaveComment #15
longwaveFirst patch includes a deprecation to check that nothing was missed. Second patch is probably the one that should be committed, as we don't strictly need to deprecate this.
Comment #17
longwaveComment #19
longwaveComment #20
hmendes commentedRe-rolled both patches.
Please review.
Comment #22
hmendes commentedSorry, adding new patches, hope its all right now.
Comment #23
longwaveI have worked on some patches in this issue but this is mostly a trivial find and replace, and the reroll looks good to me.
I think the "without deprecation" patch is the one to commit - I am not sure we should enforce strings as it might get in the way of downstream users testing with translations enabled.
Comment #24
catch@joseph.olstad this is just not how the tests are written - every browser test installs Drupal again, with a profile specified in the test. The Drupal install itself that you're running tests from has as little as possible to do with this.
If you write a test for an install profile or site where English isn't the default language, then rather than using t() you'd want to specific the correct text in the language under test.
Committed/pushed (the 'without deprecation' version - if we wanted to deprecate we'd need a follow-up issue that only does that) to 9.3.x, thanks!
This doesn't cherry-pick cleanly for 9.2.x, so needs a re-roll there.
Comment #26
catchComment #27
longwaveComment #28
catchThanks for the re-roll, looks good. Will commit this tomorrow if no objections.
Comment #29
catchCommitted/pushed to 9.2.x, thanks!