As per http://drupal.org/simpletest-tutorial-drupal7#t , I scanned the tests in the aggregator module and removed t()s from assert messages, or changed them to format_string() where appropriate.

Patch follows.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lazysoundsystem’s picture

Status: Active » Needs review
FileSize
25.39 KB

Here's the patch.

lazysoundsystem’s picture

I started this before reading through #500866: [META] remove t() from assert message, where they've been trying to solve this since 2009. Best to wait for confirmation there before putting any more work into this.

Lars Toomre’s picture

Lars Toomre’s picture

Title: Removing t() from asserts in simpletests » Remove t() from asserts messages in tests for aggregator module
Status: Needs review » Reviewed & tested by the community

I have reviewed all of the changes in this patch and they are appropriate. This will remove a number of t() calls wrapping assert text strings. Patch is also green so RTBC.

Dries’s picture

Committed to 8.x. Thanks.

Tor Arne Thune’s picture

Status: Reviewed & tested by the community » Fixed
xjm’s picture

Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

We're backporting these. :)

lazysoundsystem’s picture

Version: 8.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
FileSize
21.3 KB

And here's the backport.

lazysoundsystem’s picture

Assigned: Unassigned » lazysoundsystem
Status: Needs review » Needs work

Hang on, found some ones I missed. I'll submit another patch.

lazysoundsystem’s picture

Assigned: lazysoundsystem » Unassigned
Status: Needs work » Needs review
FileSize
21.49 KB

This one is ready for review.

amontero’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly.
Grepped for remaining t()'s on assertions and found none left.
Tentatively setting it as RTBC.

webchick’s picture

Assigned: Unassigned » jhodgdon
Issue tags: +Coding standards

Tagging, and moving to Jennifer's plate.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

RE #10 - do the additional changes you found for 7.x need to be done in Drupal 8.x too? If so, we should go back and do them in 8.x first...

So, can someone please look through the 8.x tests and verify that all of the t() asserts have been removed? If so, set this back to RTBC and I'll get it committed. If not, set this issue to 8.x and we'll need a patch there first. Thanks!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Needs review » Fixed

OK, I did the 8.x review myself, and it appears to be clean. So, I committed the above patch to 7.x, since it looks good too. Thanks all!

lazysoundsystem’s picture

Sorry, @jhodgdon, I had this on my list and was just about to get to it...

Thanks for beating me to it. And all your work checking all these rather repetitive patches.

Status: Fixed » Closed (fixed)

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

droplet’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7
FileSize
1.9 KB

During D8 translation, I found the t() used in tests.

** Let me know if it preferred to start a new issue for this fix. Thanks :)

ParisLiakos’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (fixed) » Reviewed & tested by the community
Issue tags: -Needs backport to D7

well, a new issue would be better, but since we have it..

lazysoundsystem’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorConfigurationTest.php
@@ -48,7 +48,7 @@ function testSettingsPage() {
-    $this->assertText(t('Dummy length setting'));

According to the style guide - https://drupal.org/simpletest-tutorial-drupal7#t, the first parameter here *should* have a t() call.
For assertText(), it's the second parameter (not present here) that gets output on the test result page, that we're removing them from.

jhodgdon’s picture

Status: Reviewed & tested by the community » Closed (fixed)

Yeah, the first change in this patch is incorrect. The other changes are out of scope for this issue, which is about assert messages, not about test modules. We can start another issue called "Do not use t() in modules that are set up for use in tests unless t() is being tested" if you want...