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.

CommentFileSizeAuthor
#19 1742830-19-contact-t.patch13.69 KBdcam
#12 1742830-12-t-contact.patch2.76 KBlazysoundsystem
#9 1742830-9-t-contact.patch3.22 KBlars toomre
#1 remove-t-from-Contact-asserts-1742830-1.patch14.06 KBlazysoundsystem

Comments

lazysoundsystem’s picture

lazysoundsystem commented
StatusFileSize
new remove-t-from-Contact-asserts-1742830-1.patch14.06 KB

Here's the patch.

lazysoundsystem’s picture

lazysoundsystem commented
Status: Active » Needs review

Changing to 'needs review'.

lazysoundsystem’s picture

lazysoundsystem commented

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 commented

#1: remove-t-from-Contact-asserts-1742830-1.patch queued for re-testing.

lars toomre’s picture

lars toomre commented
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

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

We're backporting these. :)

andypost’s picture

andypost
he/him
Primary language Russian
commented
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

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 1742830-9-t-contact.patch3.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

andypost
he/him
Primary language Russian
commented

@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

lars toomre commented

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

lazysoundsystem commented
StatusFileSize
new 1742830-12-t-contact.patch2.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

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Great!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » jhodgdon

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

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
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

jhodgdon
she/her
Primary language English
commented
Assigned: jhodgdon » Unassigned
andypost’s picture

andypost
he/him
Primary language Russian
commented

I think this should wait for #1588422: Convert contact categories to configuration system

dcam’s picture

dcam commented
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

dcam commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1742830-19-contact-t.patch13.69 KB

Backported #1 to D7.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -Needs backport to D7

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

izus’s picture

izus commented
Issue tags: +Needs backport to D7

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

izus’s picture

izus commented
Status: Needs review » Reviewed & tested by the community

seems good.
Thanks !

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks all! Committed to 7.x.

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