This is a sub-task of #500866: [META] remove t() from assert message focused on the database 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 database sub-system tests for the above. There are approximately 350 changes needed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
114.8 KB

Here is an initial untested patch for this issue.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

This looks good on visual spot check, and the bot approves, so yay! Thanks, Lars.

webchick’s picture

Issue tags: +Coding standards

Tagging with coding standards so Jennifer can get to this if she has time.

webchick’s picture

Assigned: Lars Toomre » jhodgdon

Might as well make that explicit.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Wow. Committed to 8.x. Should this be backported to 7.x too? And are there issues for the other tests, since this one just covers the database system tests?

Lars Toomre’s picture

Thanks @jhodgdon. I think this was my first core commit.

The main issue #500866: [META] remove t() from assert message is stalled. Correction for the comment modules needs to be rerolled in #1742602: Removing t() from asserts in simpletests in comment module. To my knowledge, there are no other active patches about removing t() from assert messages. You might want to check with @xjm though.

I can continue to roll patches like this one. However, as I asked in #500866-237: [META] remove t() from assert message, I would like some guidance on number of changes in one patch. On Monday, when locally fixing tests for Common, there were about 235 changes. Do you want that as one big group or as smaller sub-sets? Previously, @xjm was worried that large patches like this one might conflict with other active patches.

Let me know. Thanks.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

We now have a tag called "avoid commit conflicts" for issues
http://drupal.org/node/1207020#avoid-commit-conflicts
and my mandate is to avoid conflicts with those issues when I commit. Other than that, patches about the size of the one that was on this issue are fine, and you don't need to worry about the conflicts (I'll check when I do the commit). So far I haven't had any complaints about big commits messing up other patches since we introduced this tag.

Anyway, based on that base issue (which should be mentioned in this issue summary by the way!), I guess this needs a backport to D7.

dcam’s picture

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

Backported #1 to D7.

Status: Needs review » Needs work

The last submitted patch, database-1794012-8.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
104.12 KB

I mistakenly deleted a couple of t() functions that should have been changed to format_string().

dcam’s picture

#10: database-1794012-10.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I was in shock that this still applied, until I realized it's the backport patch so the code isn't really changing. :-) Bot's good with it, looks good on visual read through.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Whew, 345 lines changed! Thank goodness for diff --color-words... makes the scanning much more readable.

Thanks for the patch & review. Committed to 7.x.

dcam’s picture

Sweet. We've finally gotten one of these committed for 7.x and closed out. Thanks for the review, Crell. =)

jhodgdon, do you think these issues are good candidates for being tagged as "Novice" issues so we can maybe get some more eyes on them? They seem easy to review, although maybe a bit tedious.

jhodgdon’s picture

Many of them are already tagged Novice. Yes, definitely good novice material! We don't require Novice projects to be extremely quick, just straightforward and doable. :)

dcam’s picture

Ok, thanks. I'll check them and tag any that haven't been tagged already.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.