As per http://drupal.org/simpletest-tutorial-drupal7#t , I scanned the tests in the contact 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

Here's the patch.

lazysoundsystem’s picture

Status: Active » Needs review

Changing to 'needs review'.

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

Status: Needs review » Reviewed & tested by the community

All of the changes removing t() from assert messages are appropriate. The re-test is green too so RBTC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

xjm’s picture

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

We're backporting these. :)

andypost’s picture

Status: Patch (to be ported) » Needs work

There's a some strings are still there, also I recommend to remove a group (Contact) from asserts. Probably $this->assertEqual(value1, value2) is enough....

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.php
@@ -206,14 +206,14 @@ class ContactSitewideTest extends WebTestBase {
+    $this->assertEqual(count($captured_emails), 1, t('Auto-reply e-mail was sent to the sender for category "bar".'), 'Contact');
+    $this->assertEqual($captured_emails[0]['body'], drupal_html_to_text($bar_autoreply), t('Auto-reply e-mail body is correct for category "bar".'), 'Contact');
...
-    $this->assertEqual(count($captured_emails), 0, t('No auto-reply e-mail was sent to the sender for category "no-autoreply".'), t('Contact'));
+    $this->assertEqual(count($captured_emails), 0, t('No auto-reply e-mail was sent to the sender for category "no-autoreply".'), 'Contact');

this t() should not be there

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

Atached is a patch which addresses the missing t() removals from messages.

@andypost - Can you open up a separate issue to remove the group parameter?

andypost’s picture

@Lars Toomre, I see no reason in new issue - this one is enough.
Please, re-roll a patch without group - this lines is one used this approach and this issue is about clean-up so let's make it here

Lars Toomre’s picture

I will leave the call about whether your request is appropriate to @xjm and @jhodgdon. I am tired of being accussed of scope creap.

This isssue is primarily about removing t() which slows down tests. Whether there are or not group parameters are another issue.

lazysoundsystem’s picture

FileSize
2.76 KB

Thanks for picking these up - I noticed I'd missed them when rolling the D7 backport.

I couldn't find any documentation about how they should be used, beyond the doxygen comment that they could be 'Browser' or 'Php', so I'm not sure if 'Contact' isn't an appropriate group for these, but it is inconsistent with all of the other module tests I've been through, so here's a patch without them.

I've opened another issue to make the use of groups more clear:
#1799326: Clean up $group usage in simpletests

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!

webchick’s picture

Assigned: Unassigned » jhodgdon

Since this concerns coding standards, moving to Jennifer's queue.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Ummm... why is the group "contact" being *added* to these tests? We shouldn't be adding groups to tests.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
andypost’s picture

dcam’s picture

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

The t() functions shown in #12 were removed by http://drupal.org/node/1588422#comment-6587438. Moving to D7 for backport.

dcam’s picture

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

Backported #1 to D7.

andypost’s picture

Issue tags: -Needs backport to D7

#19: 1742830-19-contact-t.patch queued for re-testing.

izus’s picture

Issue tags: +Needs backport to D7

#19: 1742830-19-contact-t.patch queued for re-testing.

izus’s picture

Status: Needs review » Reviewed & tested by the community

seems good.
Thanks !

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.