This is a sub-task of #500866: [META] remove t() from assert message focused on the T-Z include files from the system module sub-systems (excluding system sub-system itself).

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 Theme, Update, Upgrade and Uuid sub-system tests for the above. There are approximately 98 changes needed spread out across 16 test files. This issue includes conversion of all format_patch() occurances as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
46.06 KB

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

Per #1797318-5: Remove t() from asserts from Form sub-system tests, I was under the impression that escaped strings should use format_string(). @xjm thought it was not required. Hence, in this issue, I have simply removed the t() and not changed them to format_string(). Let's see what the bot thinks.

Those assert messages in this patch also might be worthy of a closer look in the review process. We also could convert some of them to double quoted strings and remove the escape character.

dcam’s picture

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

#1 no longer applies due to changes in UpdateScriptTest.php, LanguageUpgradePathTest.php, and UpgradePathTestBase.php.

Lars Toomre’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
45.46 KB

Attached is a re-roll of #1.

dcam’s picture

Status: Needs review » Needs work

I tested #3. I found one extra t() around an assert message in Upgrade/UuidUpgradePathTest.php, line 40. If that can be added, then I'll re-test and it can be RTBC.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
46.31 KB

Here is an updated patch that includes what looks like an added assert since this was first rolled weeks ago.

Thanks for the review @dcam.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

I tested #5 and didn't find any more t()'s around assert messages. It looks good to me.

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 and ready for backport.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
59.74 KB

Backported #5 to D7.

dcam’s picture

#9: 1797920-9-t-assert-T-Z.patch queued for re-testing.

dcam’s picture

#9: 1797920-9-t-assert-T-Z.patch queued for re-testing.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

izus’s picture

Issue tags: -Novice

#9: 1797920-9-t-assert-T-Z.patch queued for re-testing.

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

The last submitted patch, 1797920-9-t-assert-T-Z.patch, failed testing.

dcam’s picture

Rerolled #9.

dcam’s picture

Status: Needs work » Needs review

Status...

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
#15 seems good for me.
Thanks

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

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

Anonymous’s picture

Issue summary: View changes

Updated initial change counts.