This is a sub-task of #500866: [META] remove t() from assert message focused on the System sub-system.

In D8 per http://drupal.org/simpletest-tutorial-drupal7#t, best practice is to remove t() from assert messages in tests. When necessary, t() should be replaced with format_string().

This issue is to correct the System sub-system tests for the above. There are approximately 127 changes needed (including format_string() changes) changing 22 Test files.

CommentFileSizeAuthor
#14 1797926-14-t-assert-system.patch47.89 KBdcam
#9 1797926-9-t-assert-system.patch56.09 KBlars toomre
#4 1797926-4-t-assert-system.patch55.82 KBlars toomre
#1 t-asset-system-1797926-1.patch55.75 KBlars toomre

Comments

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new t-asset-system-1797926-1.patch55.75 KB

Here is an initial untested patch for this issue. This patch includes format_string() conversions as well.

The format_string() conversions appear to be correct, but are worthy of a closer look in the review process. A couple of them confused me at first and I had to convert some concatenations to escaped strings.

lars toomre’s picture

lars toomre commented

Looks like #1798756: Finish conversion of error_level to CMI added another t(). That will need to be removed.

Let's first see what reviewers of this patch have to say.

dcam’s picture

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

#2 no longer applies due to changes in InfoAlterTest.php.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 1797926-4-t-assert-system.patch55.82 KB

Here is a re-roll of the patch from #1. It also includes two additional fixes from the ErrorHandlerTest.php that were referenced in #2.

I did not go back and recheck that there were no other t() around assert messages. There might well have been some added since #1 was first rolled about three weeks ago. However, #1 was complete as of its creation time.

Thanks for trying to review this @dcam. Let me know what else you think needs to be changed.

Status: Needs review » Needs work

The last submitted patch, 1797926-4-t-assert-system.patch, failed testing.

lars toomre’s picture

lars toomre commented

Drupal\file\Tests\UsageTest test failure looks like it is a random test failure.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review

#4: 1797926-4-t-assert-system.patch queued for re-testing.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I tested #4. There's an addtional t() to eliminate in SiteMaintenanceTest.php, line 103.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 1797926-9-t-assert-system.patch56.09 KB

Thanks for the review @dcam. Here is an updated patch with that one additional change.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

#9 looks good. I didn't find any additional t()'s around assert messages.

lars toomre’s picture

lars toomre commented

Thanks again for the review @dcam! I glad that we are getting the D8 patches completed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: lars toomre » jhodgdon
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed to 8.x.

dcam’s picture

dcam commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1797926-14-t-assert-system.patch47.89 KB

Backported #9 to D7.

I tried to stick to backporting only the changes made in #9. There are other t()'s around assert messages in system.test, but they may be handled by other issues with patches that are being backported. system.test will probably need to be checked for remaining t()'s after all system-test-related issues have been committed.

dcam’s picture

dcam commented
Issue tags: +Novice

Tagging as Novice.

izus’s picture

izus commented

#14: 1797926-14-t-assert-system.patch queued for re-testing.

izus’s picture

izus commented
Status: Needs review » Reviewed & tested by the community

Hi,
#14 seems good.
Thanks

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks! Committed to 7.x.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated initial counts.