Problem/Motivation

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 hundreds of calls to t() in calls to assertText() and that removing all these in one go seems to be a suitable way of attacking this problem.

Proposed resolution

Identify and remove all calls to t() wrapped in calls to assertText(), except those used by translation-related code (if any).

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#80 3145418-80-9.2.x.patch330.36 KBlongwave
#80 3145418-80-9.1.x.patch330.33 KBlongwave
#79 3145418-79.patch330.36 KBmondrake
#76 3145418-76.patch330.59 KBlongwave
#69 interdiff_54-68.txt3.71 KBmondrake
#68 interdiff_66-68.txt533 bytesshetpooja04
#68 3145418-68.patch330.59 KBshetpooja04
#66 interdiff_63-66.txt3.21 KBshetpooja04
#66 3145418-66.patch331.33 KBshetpooja04
#63 interdiff_58-63.txt3.98 KBshetpooja04
#63 3145418-63.patch333.89 KBshetpooja04
#58 interdiff_54-58.txt2.05 KBsarvjeetsingh
#58 3145418-58.patch330.57 KBsarvjeetsingh
#54 interdiff_47-54.txt1.51 KBshetpooja04
#54 interdiff_44-54.txt835 bytesshetpooja04
#54 3145418-54.patch331.3 KBshetpooja04
#52 interdiff_47-52.txt1.51 KBshetpooja04
#52 interdiff_44-52.txt835 bytesshetpooja04
#52 3145418-52.patch331.47 KBshetpooja04
#47 interdiff.3145418.44-47.txt1.02 KBlongwave
#47 3145418-47.patch333.06 KBlongwave
#44 interdiff.3145418.42-44.txt13.34 KBlongwave
#44 3145418-44.patch332.32 KBlongwave
#42 interdiff.3145418.41-42.txt7.25 KBlongwave
#42 3145418-42.patch327.31 KBlongwave
#41 interdiff_40_41.txt14.57 KBSpokje
#41 3145418-41.patch323.5 KBSpokje
#40 3145418-40.patch312.77 KBSpokje
#40 interdiff_39_40.txt878 bytesSpokje
#39 interdiff_38_39.txt962 bytesSpokje
#39 3145418-39.patch312.77 KBSpokje
#38 interdiff_34_38.txt20.82 KBSpokje
#38 3145418-38.patch312.78 KBSpokje
#35 3145418-34.patch301.91 KBnikitagupta
#34 3145418-34.patch301.91 KBnikitagupta
#31 interdiff_28-31.txt763 bytesravi.shankar
#31 3145418-31.patch305.22 KBravi.shankar
#28 interdiff.3145418.25-28.txt2.14 KBlongwave
#28 3145418-28.patch304.48 KBlongwave
#25 3145418-25.patch303.44 KBpaulocs
#24 3145418-24.patch304.97 KBpaulocs
#13 3145418-13.patch348.72 KBpaulocs
#12 3145418-12.patch308 KBpaulocs
#11 3145418-11.patch301.32 KBpaulocs
#11 interdiff-9-11.txt31.36 KBpaulocs
#9 3145418-9.patch271.95 KBpaulocs
#6 3145418-6.patch273.27 KBmrinalini9
#3 3145418-3.patch273.23 KBlongwave
#2 3145418-2.patch236.78 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
FileSize
236.78 KB

First pass with

rg -l assertText core | xargs sed -E -i "s/(assertText\()t\(('[^']+')\)/\1\2/g"

As with #3145005: [November 9, 2020] Remove uses of t() in drupalPostForm() calls there are some manual fixes required, but let's see how this runs first.

longwave’s picture

Title: Remove uses of t() in assertText() calls » Remove uses of t() in assertText() and assertNoText() calls
FileSize
273.23 KB

In fact let's expand scope slightly as these can be treated the same:

rg -l 'assert(No)?Text' core | xargs sed -E -i "s/(assert(No)?Text\()t\(('[^']+')\)/\1\3/g"
Sivaji_Ganesh_Jojodae’s picture

Status: Needs review » Needs work

The patch couldn't be applied lately.

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
273.27 KB

Rerolled patch #3, please review.

paulocs’s picture

Status: Needs review » Needs work

Patch needs re-roll.

longwave’s picture

Issue tags: +Needs reroll
paulocs’s picture

Status: Needs work » Needs review
FileSize
271.95 KB

Patch re-rolled.

longwave’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Thanks for rerolling, there are still lots of cases I didn't catch automatically with #3, these will likely need manual conversion as they need variables interpolating etc.

paulocs’s picture

Status: Needs work » Needs review
FileSize
31.36 KB
301.32 KB

Hello!

New patch with changes that had to do manually.

Edit: I don't get why patch has composer error.

Thanks

paulocs’s picture

Fixing remained assertNoText(t()).

paulocs’s picture

New patch because it was missing some.

Now are all t() calls fixed.

Thanks.

longwave’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/tests/src/Functional/BlockFormInBlockTest.php
@@ -49,7 +49,8 @@ public function testCachePerPage() {
-    $this->assertText(t('Your email address is @email', ['@email' => 'test@example.com']));
+    $email = 'test@example.com';
+    $this->assertText('Your email address is ' . $email);

We don't need to introduce a variable here. This can just become

$this->assertText('Your email address is test@example.com');

There are lots more cases like this where new variables have been introduced.

longwave’s picture

I am also worried this patch is too big to review, but I don't know how to break it down.

mondrake’s picture

paulocs’s picture

Yes, the patch is big. I took a big time to do it.
Maybe any of the maintainers could say what the best approach so we can keep working on it.
About the variables is okay, we can handle it.
Before to work on the variables, I think we should know if we will split it or not.

Cheers, Paulo.

thalles’s picture

We can divide in little patches for components/modules.

longwave’s picture

No, we explicitly aren't allowed to group by module: https://www.drupal.org/core/scope#files

longwave’s picture

As this is all tests, and if we remove the newly introduced variables then all changes will be confined to the same line, then it is probably possible to review with a word diff - the fact that it is all test coverage gives us confidence we haven't broken anything because otherwise the tests should fail.

I suppose one obvious way of breaking it up is to split assertNoText() out into its own separate issue.

Alternatively we could go back to #2 or #3 and initially fix only t() with no string replacement parameters? And then fix the string replacement variants in a followup?

paulocs’s picture

Okay. I will create a new issue for assertNoText() as a child issue from this one.

Issue #3166349: Remove uses of t() in assertNoText() was created.

longwave’s picture

Title: Remove uses of t() in assertText() and assertNoText() calls » Remove uses of t() in assertText() calls

The spinoff was committed so we can narrow scope here.

paulocs’s picture

Assigned: Unassigned » paulocs

Thanks @longwave, I forgot to edit the issue title.

I'll work on it!

paulocs’s picture

Status: Needs work » Needs review
FileSize
304.97 KB

New patch.

paulocs’s picture

Assigned: paulocs » Unassigned
FileSize
303.44 KB

Patch #24 can't be applied, so I'm attaching a new one.

Status: Needs review » Needs work

The last submitted patch, 25: 3145418-25.patch, failed testing. View results

mondrake’s picture

It would be good to temporarily add a conditional deprecation so that when a t() object is passed in, a deprecation error is thrown. This would give us confidence that there are no more cases left out. Since we do not want to be strict here and drop passing t() altogether, when confirmed the above we can remove the temporary deprecation from the patch, before RTBC.

longwave’s picture

Status: Needs work » Needs review
FileSize
304.48 KB
2.14 KB

Fixed test fail, also added the temporary deprecation suggested in #27.

Status: Needs review » Needs work

The last submitted patch, 28: 3145418-28.patch, failed testing. View results

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar

Working on this.

ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
FileSize
305.22 KB
763 bytes

Here I have tried to fix failed tests.

Vidushi Mehta’s picture

Status: Needs work » Needs review
ravi.shankar’s picture

Status: Needs review » Needs work

Setting status to NW as patch #31 didn't pass the tests.

nikitagupta’s picture

Status: Needs work » Needs review
FileSize
301.91 KB
nikitagupta’s picture

FileSize
301.91 KB

Status: Needs review » Needs work

The last submitted patch, 35: 3145418-34.patch, failed testing. View results

longwave’s picture

@nikitagupta please upload interdiffs and write comments as to what you changed when you upload new patches - when patches are hundreds of kb as here it is very hard to figure out what has changed otherwise.

Spokje’s picture

Assigned: Unassigned » Spokje
FileSize
312.78 KB
20.82 KB
Spokje’s picture

Spokje’s picture

Spokje’s picture

Status: Needs review » Needs work

The last submitted patch, 42: 3145418-42.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

Status: Needs work » Needs review
FileSize
332.32 KB
13.34 KB

Hopefully this covers all the remaining ones.

mondrake’s picture

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -73,6 +73,9 @@ protected function assertElementNotPresent($css_selector) {
+    if (is_object($text)) {
+      @trigger_error('Passing MarkupInterface objects to AssertLegacyTrait::assertText() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Pass plain strings instead. See https://www.drupal.org/node/xxxxxxx', E_USER_DEPRECATED);
+    }

Can we do the same for assertNoText, just to make sure in the peer issue #3166349: Remove uses of t() in assertNoText() we have not missed anything?

mondrake’s picture

longwave’s picture

Sure, though if this fails badly, due to the size of the patch I think we should move fixing this to a followup.

Status: Needs review » Needs work

The last submitted patch, 47: 3145418-47.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

#47 thanks, agree to fix the couple of residual instnces in a follow up is better.

Let’s remove the @trigger_error from #44 here then, and this is RTBC.

longwave’s picture

Issue tags: +Needs change record

Needs a change record to link the deprecation message to.

mondrake’s picture

My suggestion is to just remove all this change:

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -73,6 +73,9 @@ protected function assertElementNotPresent($css_selector) {
    */
   protected function assertText($text) {
     @trigger_error('AssertLegacyTrait::assertText() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseContains() or $this->assertSession()->pageTextContains() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
+    if (is_object($text)) {
+      @trigger_error('Passing MarkupInterface objects to AssertLegacyTrait::assertText() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Pass plain strings instead. See https://www.drupal.org/node/xxxxxxx', E_USER_DEPRECATED);
+    }
     // Cast MarkupInterface to string.
     $text = (string) $text;
 
@@ -113,6 +116,9 @@ protected function assertText($text) {

@@ -113,6 +116,9 @@ protected function assertText($text) {
    */
   protected function assertNoText($text) {
     @trigger_error('AssertLegacyTrait::assertNoText() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseNotContains() or $this->assertSession()->pageTextNotContains() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
+    if (is_object($text)) {
+      @trigger_error('Passing MarkupInterface objects to AssertLegacyTrait::assertNoText() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Pass plain strings instead. See https://www.drupal.org/node/xxxxxxx', E_USER_DEPRECATED);
+    }
     // Cast MarkupInterface to string.
     $text = (string) $text;
 

In #27 I was suggesting to add it while developing the patch to automate checking we address all the conversions, but I am not sure we must enforce this permanently.

assertText forces casting the argument to string:

  protected function assertText($text) {
    @trigger_error('AssertLegacyTrait::assertText() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseContains() or $this->assertSession()->pageTextContains() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
    // Cast MarkupInterface to string.
    $text = (string) $text;
    ...

If we do that we do not need a CR.

shetpooja04’s picture

Status: Needs work » Needs review
FileSize
331.47 KB
835 bytes
1.51 KB

Removing the @trigger_error from #44 as mentioned in #49 and #51

mondrake’s picture

Status: Needs review » Needs work
shetpooja04’s picture

Status: Needs work » Needs review
FileSize
331.3 KB
835 bytes
1.51 KB

Removing the @trigger_error from #44 as mentioned in #49 and #51

mondrake’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record
  1. +++ b/core/modules/field_ui/tests/src/Functional/ManageFieldsFunctionalTest.php
    @@ -426,7 +426,7 @@ public function testFieldPrefix() {
    -    $this->assertText(new FormattableMarkup('@label settings for @type', ['@label' => $this->fieldLabel, '@type' => $this->contentType]));
    +    $this->assertSession()->fieldValueEquals('label', $this->fieldLabel);
    

    This does not look quite right.

  2. +++ b/core/modules/tracker/tests/src/Functional/TrackerNodeAccessTest.php
    @@ -59,11 +59,11 @@ public function testTrackerNodeAccess() {
         // Create some nodes.
         $private_node = $this->drupalCreateNode([
    -      'title' => t('Private node test'),
    +      'title' => 'Private node test',
           'private' => TRUE,
         ]);
         $public_node = $this->drupalCreateNode([
    -      'title' => t('Public node test'),
    +      'title' => 'Public node test',
           'private' => FALSE,
         ]);
     
    

    this seems out-of-scope here.

mondrake’s picture

mondrake’s picture

Also, there is one coding standard issue.

sarvjeetsingh’s picture

Status: Needs work » Needs review
FileSize
330.57 KB
2.05 KB

Updated patch after making changes as per #55 and #57.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 3145418-58.patch, failed testing. View results

longwave’s picture

In ManageFieldsFunctionalTest I couldn't get the test to work when asserting plain text, the <em> markup was getting in the way. I figured that all it is really looking for is that the field is correctly created, and asserting the label field seemed to be a cleaner way of doing this than looking for the page title.

I think TrackerNodeAccessTest would also fail if we put the deprecation back in:

    $this->assertText($private_node->getTitle(), 'Private node is visible to user with private access.');
    $this->assertText($public_node->getTitle(), 'Public node is visible to user with private access.');

When t() is used to create the nodes, both titles are TranslatableMarkup instead of bare strings.

mondrake’s picture

-    $this->assertText(new FormattableMarkup('@label settings for @type', ['@label' => $this->fieldLabel, '@type' => $this->contentType]));
+    $this->assertText($this->fieldLabel . ' settings for ' . $this->contentType); 

I think here we can use straight $this->assertSession()->pageTextContains(...) that strips the tags from the response before making the check.

shetpooja04’s picture

Status: Needs work » Needs review
FileSize
333.89 KB
3.98 KB

Uploading patch for the points mentioned in comments #61 and #62
Please review

mondrake’s picture

Status: Needs review » Needs work

I would revert all of the TrackerNodeAccessTest changes in #63 - it's not changing the essence of passing a translatable object to drupalCreateNode, and the second argument in assertText must be removed anyway in #3159788: AssertLegacyTrait::assert(No)Text() in functional tests still have a message passed in.

+++ b/core/modules/tracker/tests/src/Functional/TrackerNodeAccessTest.php
@@ -6,6 +6,7 @@
 use Drupal\node\Entity\NodeType;
 use Drupal\Tests\BrowserTestBase;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
 
 /**
  * Tests for private node access on /tracker.
@@ -15,6 +16,7 @@

@@ -15,6 +16,7 @@
 class TrackerNodeAccessTest extends BrowserTestBase {
 
   use CommentTestTrait;
+  use StringTranslationTrait;
 
   /**
    * Modules to enable.
@@ -59,30 +61,30 @@ public function testTrackerNodeAccess() {

@@ -59,30 +61,30 @@ public function testTrackerNodeAccess() {
 
     // Create some nodes.
     $private_node = $this->drupalCreateNode([
-      'title' => t('Private node test'),
+      'title' => $this->t('Private node test'),
       'private' => TRUE,
     ]);
     $public_node = $this->drupalCreateNode([
-      'title' => t('Public node test'),
+      'title' => $this->t('Public node test'),
       'private' => FALSE,
     ]);
 
     // User with access should see both nodes created.
     $this->drupalGet('activity');
-    $this->assertText($private_node->getTitle(), 'Private node is visible to user with private access.');
-    $this->assertText($public_node->getTitle(), 'Public node is visible to user with private access.');
+    $this->assertText($private_node->getTitle(), $this->t('Private node is visible to user with private access.'));
+    $this->assertText($public_node->getTitle(), $this->t('Public node is visible to user with private access.'));
     $this->drupalGet('user/' . $access_user->id() . '/activity');
-    $this->assertText($private_node->getTitle(), 'Private node is visible to user with private access.');
-    $this->assertText($public_node->getTitle(), 'Public node is visible to user with private access.');
+    $this->assertText($private_node->getTitle(), $this->t('Private node is visible to user with private access.'));
+    $this->assertText($public_node->getTitle(), $this->t('Public node is visible to user with private access.'));
 
     // User without access should not see private node.
     $this->drupalLogin($no_access_user);
     $this->drupalGet('activity');
-    $this->assertNoText($private_node->getTitle(), 'Private node is not visible to user without private access.');
-    $this->assertText($public_node->getTitle(), 'Public node is visible to user without private access.');
+    $this->assertNoText($private_node->getTitle(), $this->t('Private node is not visible to user without private access.'));
+    $this->assertText($public_node->getTitle(), $this->t('Public node is visible to user without private access.'));
     $this->drupalGet('user/' . $access_user->id() . '/activity');
-    $this->assertNoText($private_node->getTitle(), 'Private node is not visible to user without private access.');
-    $this->assertText($public_node->getTitle(), 'Public node is visible to user without private access.');
+    $this->assertNoText($private_node->getTitle(), $this->t('Private node is not visible to user without private access.'));
+    $this->assertText($public_node->getTitle(), $this->t('Public node is visible to user without private access.'));
   }

#61 then I'm even more convinced to remove the deprecation trigger here, it would be too strict. If we end up having assertions where there's mistmatch between the arguments in terms of tags included or not, we can address in the final #3139404: Replace usages of AssertLegacyTrait::assertText, that is deprecated.

mondrake’s picture

Assigned: Spokje » Unassigned
shetpooja04’s picture

Status: Needs work » Needs review
FileSize
331.33 KB
3.21 KB

Reverting changes made in TrackerNodeAccessTest as mentioned in comment #64

mondrake’s picture

Status: Needs review » Needs work

We need to revert this one too.

+++ b/core/modules/tracker/tests/src/Functional/TrackerNodeAccessTest.php
@@ -59,11 +59,11 @@ public function testTrackerNodeAccess() {
     // Create some nodes.
     $private_node = $this->drupalCreateNode([
-      'title' => t('Private node test'),
+      'title' => 'Private node test',
       'private' => TRUE,
     ]);
     $public_node = $this->drupalCreateNode([
-      'title' => t('Public node test'),
+      'title' => 'Public node test',
       'private' => FALSE,
     ]);
shetpooja04’s picture

Status: Needs work » Needs review
FileSize
330.59 KB
533 bytes

Reverted the changes according to #67

mondrake’s picture

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

Thank you! Adding interdiff between #54 and #68.

xjm’s picture

Status: Reviewed & tested by the community » Postponed
mondrake’s picture

Status: Postponed » Reviewed & tested by the community
xjm’s picture

@mondrake Oops sorry, got the xpath tab mixed in with the ones #3157960: Replace t() calls with $this->t() in ToolbarAdminMenuTest.php and ToolbarMenuTranslationTest.php. Thanks for catching it.

xjm’s picture

Title: Remove uses of t() in assertText() calls » [NEEDS SCHEDULING] Remove uses of t() in assertText() calls
Issue tags: +beta target

Once a committer has reviewed this, we should schedule it as a beta target (sometime about the second week of November).

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Pasqualle’s picture

Version: 9.2.x-dev » 9.1.x-dev
longwave’s picture

Rerolled. Hopefully this can be scheduled for early next week?

mondrake’s picture

Title: [NEEDS SCHEDULING] Remove uses of t() in assertText() calls » [November 9, 2020] Remove uses of t() in assertText() calls
mondrake’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
mondrake’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
330.36 KB

rerolled

longwave’s picture

#79 is good for 9.2.x but 9.1.x has diverged slightly so we need separate patches.

  • catch committed 5939e6b on 9.2.x
    Issue #3145418 by longwave, paulocs, shetpooja04, Spokje, nikitagupta,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and 9.1.x, thanks!

  • catch committed b308e02 on 9.1.x
    Issue #3145418 by longwave, paulocs, shetpooja04, Spokje, nikitagupta,...

Status: Fixed » Closed (fixed)

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