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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
55.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

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

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

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

Lars Toomre’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
55.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

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

Lars Toomre’s picture

Status: Needs work » Needs review

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

dcam’s picture

Status: Needs review » Needs work

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

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
56.09 KB

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

dcam’s picture

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

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

webchick’s picture

Assigned: Lars Toomre » jhodgdon
jhodgdon’s picture

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

Status: Patch (to be ported) » Needs review
FileSize
47.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

Issue tags: +Novice

Tagging as Novice.

izus’s picture

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

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
#14 seems good.
Thanks

jhodgdon’s picture

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

Issue summary: View changes

Updated initial counts.