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 assertFieldByName(), assertFieldByXpath() and assertNoFieldByXPath() 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 assertFieldByName(), assertFieldByXpath() and assertNoFieldByXPath(), except those used by translation-related code (if any).

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Hardik_Patel_12 created an issue. See original summary.

hardik_patel_12’s picture

Status: Active » Needs review
StatusFileSize
new3.65 KB

Kindly review the patch.

mondrake’s picture

Issue tags: +Deprecated assertions
mohrerao’s picture

StatusFileSize
new9.77 KB

Reroll # 2 as patch failed to apply

siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale

Hi, I have tested the issue and the patch applies cleanly. Attaching the test result.

$ vendor/bin/phpunit -c core/ --filter ExposedFormTest
PHPUnit 8.5.5 by Sebastian Bergmann and contributors.

Testing
.........                                                           9 / 9 (100%)

Time: 4.58 minutes, Memory: 833.00 MB

OK (9 tests, 137 assertions)
$ vendor/bin/phpunit -c core/ --filter DefaultViewsTest
PHPUnit 8.5.5 by Sebastian Bergmann and contributors.

Testing
.....                                                               5 / 5 (100%)

Time: 2.75 minutes, Memory: 833.00 MB

OK (5 tests, 228 assertions)
$ vendor/bin/phpunit -c core/ --filter ExposedFormUITest                                                                                                                                            130 ↵
PHPUnit 8.5.5 by Sebastian Bergmann and contributors.

Testing
....                                                                4 / 4 (100%)

Time: 2.65 minutes, Memory: 833.00 MB

OK (4 tests, 252 assertions)
siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 3153186_reroll-4.patch, failed testing. View results

mohrerao’s picture

Status: Needs work » Needs review

Failing test passes in local. Looks like a random javascript test failure. Queuing for test again.

mondrake’s picture

Status: Needs review » Needs work

#4 does not seem to have anything in common with #2.

Also 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. When confirmed, then, we can remove the temporary deprecation from the patch before RTBC.

snehalgaikwad’s picture

Status: Needs work » Needs review
StatusFileSize
new3.59 KB

Removed uses of t() from assertFieldByName(), assertFieldByXpath() and assertNoFieldByXPath() functions.

longwave’s picture

Title: Remove uses of t() in assertFieldByName(), assertFieldByXpath() and assertNoFieldByXPath() calls » Remove uses of t() in assert[No]FieldBy*() calls
StatusFileSize
new19.79 KB

Added the suggested test deprecation, found several missing cases, increased scope to all assert[No]FieldBy* methods (as they all eventually call the XPath methods, and that is where the test deprecation lives)

longwave’s picture

StatusFileSize
new17.4 KB

Forgot the interdiff

Status: Needs review » Needs work

The last submitted patch, 11: 3153186-11.patch, failed testing. View results

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar

Working on this.

ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new20.54 KB
new770 bytes

Here I have tried to fix the failed test of patch #11.

Status: Needs review » Needs work

The last submitted patch, 15: 3153186-15.patch, failed testing. View results

hardik_patel_12’s picture

AssertLegacyTrait::assertFieldByXPath() is deprecated in drupal:9.1.0 , kindly see https://www.drupal.org/node/3129738

snehalgaikwad’s picture

Status: Needs work » Needs review
StatusFileSize
new20.81 KB
new7.25 KB

Replaced deprecated AssertLegacyTrait::assertFieldByXPath() and AssertLegacyTrait::assertNoFieldByXPath() functions.

longwave’s picture

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

#18 doesn't apply.

snehalgaikwad’s picture

Assigned: Unassigned » snehalgaikwad
snehalgaikwad’s picture

Assigned: snehalgaikwad » Unassigned
Status: Needs work » Needs review
StatusFileSize
new20.83 KB
longwave’s picture

Status: Needs review » Needs work

#21 doesn't apply.

neslee canil pinto’s picture

Status: Needs work » Needs review
StatusFileSize
new20.62 KB

Status: Needs review » Needs work

The last submitted patch, 23: 3153186-23.patch, failed testing. View results

nikitagupta’s picture

Status: Needs work » Needs review
StatusFileSize
new20.49 KB
new10.17 KB
nikitagupta’s picture

StatusFileSize
new12.83 KB
new16.68 KB

Status: Needs review » Needs work

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

megha_kundar’s picture

Assigned: Unassigned » megha_kundar
megha_kundar’s picture

Assigned: megha_kundar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new15.35 KB

Status: Needs review » Needs work

The last submitted patch, 29: 3153186-29.patch, failed testing. View results

megha_kundar’s picture

Status: Needs work » Needs review
StatusFileSize
new15.28 KB
longwave’s picture

Issue tags: -Needs reroll

@Megha_kundar please upload interdiffs when working on patches, and comment to say what you have changed, as it helps reviewers figure out what has been done.

mondrake’s picture

I am not sure about the scope here - most of the assert[No]FieldBy*() calls have been converted to WebAssert already, and converrsion of assert[No]FieldByXPath() is ready for review at #3139429: Replace usages of AssertLegacyTrait::assert(No)FieldByXPath, that is deprecated - if that goes in first this issue will have no more material.

How about postponing on that, and then rescope this issue to fix assertSession()->fieldValue[Not]Equals instead?

Status: Needs review » Needs work

The last submitted patch, 31: 3153186-30.patch, failed testing. View results

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.

longwave’s picture

Status: Needs work » Closed (duplicate)

Closing as duplicate, there are no instances remaining if I grep for assert(No)?FieldBy.*t\(