[meta] Remove usage of t() in tests not testing translation

Thanks for opening this! Note that the estimate in the summary was from @longwave in Slack. I was just pasting it into the other issue. Updating summary to give credit where due.

Given the huge # of required changes here, we might need to convert this into a meta and split up the task by core module or subsystem or something. TBD.

Now the scope matters, IMHO, before proceeding, to get a confirmation about the scope from committer(s) is important. Filing issue per module/per component is not encouraged according to the current scope policy, or issues filed will be rejected/marked as won't fix, that would be a waste of time to contributors.

Thanks!

Could we scope this by method call? e.g. removing t() from drupalPostForm() would seem to be fairly straightforward to do and review, and that would cover a large number of the calls discovered in #2.

2234 changes that mostly look correct just with

grep -rl drupalPostForm core | xargs sed -E -i "s/(drupalPostForm.* )t\(('[^']+')\)/\1\2/g"

I guess the translations with arguments should be replaced with new FormattableMarkup()

+1 to #9. Should we move that to the first child issue and make this a sub-meta? ;)

Opened #3145005: [November 9, 2020] Remove uses of t() in drupalPostForm() calls to kick this off.

Also opened #3145418: [November 9, 2020] Remove uses of t() in assertText() calls but I think I should wait for feedback on this scoping until we go further down this path, there is a long tail after this:

$ grep -o '>[a-zA-Z]*' remove_t_list.txt |sort|uniq -c|sort -rn|head -40
   2188 >drupalPostForm
   1257 >
    556 >assertText
    351 >assertRaw
    239 >id
    160 >clickLink
    146 >assertNoText
     98 >assertEqual
     46 >t
     46 >assertLink
     42 >getMessage
     39 >assertTitle
     36 >label
     34 >toString
     34 >assertNoRaw
     31 >getType
     26 >assertTrue
     23 >assertSame
     20 >getAccountName

There is no need to use t() in tests

There's already in issue open for this ... #2552067: Remove t() from assertion messages in tests ... has been open for five years.

It's also something that has been addressed time and again for more than 10 years, e.g.:
#500866: [META] remove t() from assert message
#2555145: Remove t() from pass/fail assertions in tests
... and probably a dozen others.

Rather than continuing to expend effort repeatedly trying to correct this, shouldn't we develop a sniff and then be done with it?

Combined with the comment #10 and the comment #14. I would suggest

1. Round 1: Autofix -- Create a sniff to disallow using t() in tests and make it auto-fixible by phpcbf
2. Round 2: Handling false positives from Round 1 -- Change t() to new FormattableMarkup() if possible
3. Round 3: Further handling false positives from Round 1 -- Ignore coding standard checks to t() usages where t() has to be called by using phpcs:ignore

If I/we install Drupal in another language using these changes and run these tests, the tests will fail.

Each test installs a new site based on a test-specific profile; they do not use the existing configuration and therefore always run in the default language.

ah ok, makes sense then. Thanks for the explanation

however ya, it might be useful to test profile installations in another language at some point?
not sure if the performance gains are worth it to make these changes, it is not that expensive.

@joseph.olstad: All those things have been discussed again and again for >10 years. PLEASE let's not re-open that - it was decided long ago that this is the right thing to do. And it has been documented as the core policy for all this time. For example in:
https://www.drupal.org/docs/testing/phpunit-in-drupal/phpunit-browser-te...

The problem we have here is that t() keeps getting used in new tests, to a point where there are thousands of usages again. We need a way to make sure this STAYS fixed if we are going to put in all the work trying to fix it again.

This current issue is about "Remov(ing) usage of t() in tests not testing translation" - no one is saying that you can't use t() when testing translation-related things, but also be aware that testing the translation system is something that should be done in translation system tests, not in every single test of every single feature in core and contrib.

I'm all for removing t() from tests but #3153168: Remove uses of t() in setLabel() and setDescription() calls is different. It's removing it from modules that support tests. I'm not sure the case is as clear cut there. People learn from this code and use copy&paste. I think we should exclude such fixtures from this meta. See #3157938: Use t() for #title and #description in tests and test modules for an issue that wants to add t() to such code.

We've done nearly 75% of these cases now, there were 4247 when we started:

$ find core -type f -iname '*Test.php' | xargs grep '[^a-zA-Z]t(' | wc -l
1105

Woo hoo, down to 1035.

$ find core -type f -iname '*Test.php' | xargs grep '[^a-zA-Z]t(' | wc -l
1035

One way of tackling the remainder might be to try and handle all t() that use only plain strings (no replacement values) in a single issue:

$ find core -type f -iname '*Test.php' | xargs grep -E '\Wt\([^%!@:]+\)' | wc -l
565

If we ignore assertEquals() which is being dealt with in #3153468: Strip HTML tags when using assertEquals() to compare markup we are left with 279, and many of these are false positives:

$ find core -type f -iname '*Test.php' | xargs grep '[^a-zA-Z]t(' | grep -v assertEqual | wc -l
279

We are down to 201 remaining instances:

$ find core -type f -iname '*Test.php' | xargs grep '[^a-zA-Z]t(' | wc -l
201

A lot of the remaining ones are %-placeholdered strings, e.g.

$this->assertEquals(t('Field %field set in %filter is not set in display %display.', ['%field' => 'dummy', '%filter' => 'Global: Combine fields filter', '%display' => 'Default']), reset($errors['default']));

where we can convert to either

$this->assertEquals('Field <em class="placeholder">dummy</em> set in <em class="placeholder">Global: Combine fields filter</em> is not set in display <em class="placeholder">Default</em>.', reset($errors['default']));

or

$this->assertEquals('Field dummy set in Global: Combine fields filter is not set in display Default.', strip_tags(reset($errors['default'])));

Not sure which is preferable.

Tagging for coding standards and moving to the 'other' component where such issues live.

#3337295: Remove remaining uses of t() in assertEquals() calls landed.

I think we are down to the final few stragglers, opened #3410128: Remove remaining unnecessary uses of t() in tests to cover those.

I also am leaning towards closing #3153168: Remove uses of t() in setLabel() and setDescription() calls as won't fix as we are effectively testing that APIs can receive TranslatableMarkup, as they would outside of tests.

Closed that related issue and moved credit here

And the number of instances has risen since #30.

$ find core -type f -iname '*Test.php' | xargs grep '[^a-zA-Z]t(' | wc -l
274

Trying to confirm that this is done, since #3410128: Remove remaining unnecessary uses of t() in tests has been committed. I searched Test files using the grep in that issue and ignored all the files listed in that IS that are valid uses of t(). Then ignored the tests in package_manager because which isn't testing translations but tests validation messages where the methods have a type hint for TranslatableMarkup. That left 16 files with 26 instances of t() that need to be checked.

1. core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php
2. core/modules/file/tests/src/Functional/FileManagedFileElementTest.php
3. core/modules/language/tests/src/Functional/LanguageUILanguageNegotiationTest.php
4. core/modules/locale/tests/src/Functional/LocaleTranslatedSchemaDefinitionTest.php
5. core/modules/migrate/tests/modules/migrate_high_water_test/src/Plugin/migrate/source/HighWaterTest.php
6. core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
7. core/modules/navigation/tests/src/Functional/NavigationLogoTest.php
8. core/modules/navigation/tests/src/Unit/TopBarItemManagerTest.php
9. core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
10. core/modules/system/tests/src/Kernel/System/RunTimeRequirementsTest.php
11. core/modules/views/tests/src/Kernel/Handler/FieldKernelTest.php
12. core/modules/views_ui/tests/src/Unit/ViewListBuilderTest.php
13. core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
14. core/tests/Drupal/KernelTests/Core/Updater/UpdateRequirementsTest.php
15. core/tests/Drupal/KernelTests/Core/Url/LinkGenerationTest.php
16. core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php

Of those, 13, is testing translations. The rest includes some that were missed in #3410128: Remove remaining unnecessary uses of t() in tests and others from commits made this month. There is nothing to prevent added other unnecessary uses of t() in tests so lets' fix this last group and stop chasing new introduced cases. Right now, we can only rely on review to prevent the addition of more unnecessary uses of t() in tests.

Here is the script I used for finding usages of t() in tests.

Postponing until the issues to fix "DrupalPractice.Objects.GlobalFunction" in tests is complete. #3113904: [META] Replace t() calls inside of classes

Used the script provided and believe all instances have been found.

This appears to be removing one assertion entirely instead of updating it to not use t().

Work is not needed, the line is a duplicate.

Committed e4df448 and pushed to 11.x. Thanks!

Amending attribution.