Posted by xjm

Part of meta issue #500866: [META] remove t() from assert message focused on the Search module.

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().

There are approximately XXX changes needed in XX 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

Assigned: xjm » Lars Toomre

An initial untest patch for this one is forthcoming shortly!

xjm’s picture

Oh, I had actually already generated part of the patch, which is why I'd assigned the issue to myself (as with the others). But it wasn't a lot of work, so no big deal.

Lars Toomre’s picture

Title: Remove t() from assertion messages in tests for the search module » Remove t() from assertion messages in tests for the Search module
Status: Active » Needs review
FileSize
19.03 KB

Here is an initial untested patch for this issue. It includes a number of format_string() conversions.

It also includes some string concatenations that were turned into format_string() as well. Please take a closer look at those to make sure I did the conversions correctly.

Status: Needs review » Needs work

The last submitted patch, 1797508-2-t-search.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
18.03 KB

A slight formatting problem should be fixed with this untested patch.

Status: Needs review » Needs work

The last submitted patch, 1797508-5-t-search.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
18.04 KB

Helps if I remember to put the placeholders in an array!

dcam’s picture

Status: Needs review » Reviewed & tested by the community

#7 looks good to me. I didn't find any remaining t()'s in the Search module test asserts. Marking as RTBC.

webchick’s picture

Assigned: Lars Toomre » jhodgdon

Tum te tum...

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x, thanks all! Ready for port...

dcam’s picture

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

Backported #7 to D7.

dcam’s picture

#11: 1797508-11-t-search.patch queued for re-testing.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

theladebug’s picture

Maybe I'm misunderstanding, but there are obviously t()'s in the patch in #11.

jhodgdon’s picture

To review this patch, what you want to do is:
- Apply the patch.
- Verify that after the patch is applied, there are no t() messages left around test assertion messages in the affected files.
- Verify by using the appropriate git diff command that the changes made are accurate.

The base issue has more information:
#500866: [META] remove t() from assert message

dcam’s picture

If you would like information on reviewing these patches, I'm happy to help out. If you're sprinting in Portland today, come find me (dcam). I'm directing traffic out in front of the main core contributor sprint rooms, B113-119. Look for the guy in the green shirt and black hoodie.

jhodgdon’s picture

#11: 1797508-11-t-search.patch queued for re-testing.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

lazysoundsystem’s picture

Status: Needs review » Reviewed & tested by the community

This too looks good. RTBC.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all! Committed to 7.x.

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

Anonymous’s picture

Issue summary: View changes

Removing myself from the author field so I can unfollow. --xjm