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 severals of calls to t() in calls to linkExists() and linkNotExists() 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 linkExists() and linkNotExists(), except those used by translation-related code (if any).

Create a list of Test files using t() by find core -type f -iname '*Test.php' | xargs grep '[^a-zA-Z]t(' > remove_t_list.txt, then search that list for linkExists and linkNotExists.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#25 3153143-25.patch40.06 KBHardik_Patel_12
#18 interdiff_16-18.txt2.41 KBridhimaabrol24
#18 3153143-18.patch40.03 KBridhimaabrol24
#16 interdiff_12-16.txt5.32 KBridhimaabrol24
#16 3153143-16.patch40.15 KBridhimaabrol24
#12 interdiff_7-12.txt10.22 KBsiddhant.bhosale
#12 3153143-12.patch34.09 KBsiddhant.bhosale
#7 3153143-7.patch30.54 KBguptahemant
#2 3153143-2.patch30.58 KBHardik_Patel_12
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hardik_Patel_12 created an issue. See original summary.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
Status: Active » Needs review
Parent issue: » #3133726: [meta] Remove usage of t() in tests not testing translation
FileSize
3153143-2.patch30.58 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3153143-2.patch, failed testing. View results

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
Issue tags: +Deprecated assertions
guptahemant’s picture

guptahemant CreditAttribution: guptahemant as a volunteer and at QED42 for Drupal India Association commented
Issue summary: View changes
guptahemant’s picture

guptahemant CreditAttribution: guptahemant as a volunteer and at QED42 for Drupal India Association commented
Assigned: Unassigned » guptahemant
guptahemant’s picture

guptahemant CreditAttribution: guptahemant as a volunteer and at QED42 for Drupal India Association commented
Assigned: guptahemant » Unassigned
FileSize
3153143-7.patch30.54 KB

Here is an updated patch.

guptahemant’s picture

guptahemant CreditAttribution: guptahemant as a volunteer and at QED42 for Drupal India Association commented
Status: Needs work » Needs review
meena.bisht’s picture

meena.bisht CreditAttribution: meena.bisht at QED42 for Drupal India Association commented
Assigned: Unassigned » meena.bisht
apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno commented
Assigned: meena.bisht » Unassigned
siddhant.bhosale’s picture

siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commented
Assigned: Unassigned » siddhant.bhosale

Hi, I found changes left in files. eg.
core/modules/comment/tests/src/Functional/CommentLinksAlterTest.php has the t() used.
$this->assertSession()->linkExists(t('Report'));. Marking this to needs works.

siddhant.bhosale’s picture

siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commented
FileSize
3153143-12.patch34.09 KB
interdiff_7-12.txt10.22 KB

Hi, I have made the changes. Adding the patch and interdiff as well.

siddhant.bhosale’s picture

siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commented
Assigned: siddhant.bhosale » Unassigned
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: -Deprecated assertions
quietone’s picture

quietone CreditAttribution: quietone commented
Status: Needs review » Needs work

Applied the patch. Then ran grep -r '\->\(linkExists\|linkNotExists\)(t(' core | nl and 11 more instances were found.

ridhimaabrol24’s picture

ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3153143-16.patch40.15 KB
interdiff_12-16.txt5.32 KB

Removing the remaining t() calls. Please review.

Status: Needs review » Needs work

The last submitted patch, 16: 3153143-16.patch, failed testing. View results

ridhimaabrol24’s picture

ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3153143-18.patch40.03 KB
interdiff_16-18.txt2.41 KB

Fixing failed test cases.

samiullah’s picture

samiullah CreditAttribution: samiullah at Salsa Digital commented

@ridhimaabrol24 some bit of code still has t() after applying ur patch

sami@Samis-MacBook-Air drupal % git grep '\->\(linkExists\|linkNotExists\)(t('
core/modules/book/tests/src/Functional/BookTest.php:    $this->assertSession()->linkNotExists(t('Book outline'), 'Book Author is not allowed to outline');
core/modules/book/tests/src/Functional/BookTest.php:    $this->assertSession()->linkNotExists(t('Remove from book outline'));
samiullah’s picture

samiullah CreditAttribution: samiullah at Salsa Digital commented
Status: Needs review » Needs work
siddhant.bhosale’s picture

siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commented
Assigned: Unassigned » siddhant.bhosale
ridhimaabrol24’s picture

ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commented

Hi @samiullah
By applying patch #18 there are no instances left of linkExists(t( and linkNotExists(t(
Please recheck the same.
Also the code you are referring to in #19 is already present in the patch.
Let me know if you still find issues.
Thanks

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes

I applied that patch and found no usages of t() in lines using linkNotExists or linkExists.

+++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationOverviewTest.php
@@ -126,7 +126,7 @@ public function testMapperListPage() {
+      $this->assertSession()->linkExists('Translate test configuration');

+++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php
@@ -299,7 +299,7 @@ public function testSourceValueDuplicateSave() {
+    $this->assertSession()->linkNotExists('Edit');

@@ -331,7 +331,7 @@ public function testContactConfigEntityTranslation() {
+    $this->assertSession()->linkExists('Translate contact form');

@@ -339,7 +339,7 @@ public function testContactConfigEntityTranslation() {
+      $this->assertSession()->linkExists('Translate contact form');

@@ -516,10 +516,10 @@ public function testAccountSettingsConfigurationTranslation() {
+    $this->assertSession()->linkExists('Translate account settings');
...
+    $this->assertSession()->linkExists('Translate account settings');

+++ b/core/modules/menu_link_content/tests/src/Functional/MenuLinkContentTranslationUITest.php
@@ -73,7 +73,7 @@ protected function createEntity($values, $langcode, $bundle_name = NULL) {
+    $this->assertSession()->linkNotExists('Translate');

However, all of these are in tests related to translations. Are we sure the t() should be removed in all these cases? I don't know and I haven't looked at the tests myself.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Issue tags: +Needs reroll

Patch needs re-roll.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
Assigned: siddhant.bhosale » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3153143-25.patch40.06 KB

Re-rolling for 9.1.x , kindly review.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs review » Reviewed & tested by the community

Hello @all,

Patch #25 looks good. I did not find any linkExists() and linkNotExists() with a t() inside.

About comment #23, is okay to remove it because it is testing if the link with the specified label exists or not. So it makes no sense to remove it.
See https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/phpunit-browser-test-tutorial#t

Set to RTBC.

Cheers, Paulo.

Status: Reviewed & tested by the community » Needs work

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Reviewed & tested by the community

Random fail, back to RTBC.

  • catch committed 4a7c205 on 9.1.x 
    Issue #3153143 by ridhimaabrol24, Hardik_Patel_12, siddhant.bhosale,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 4a7c205 and pushed to 9.1.x. Thanks!

samiullah’s picture

samiullah CreditAttribution: samiullah at Salsa Digital commented

Looks good now @catch

Status: Fixed » Closed (fixed)

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