Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 1742830-19-contact-t.patch | 13.69 KB | dcam |
#12 | 1742830-12-t-contact.patch | 2.76 KB | lazysoundsystem |
#9 | 1742830-9-t-contact.patch | 3.22 KB | Lars Toomre |
#1 | remove-t-from-Contact-asserts-1742830-1.patch | 14.06 KB | lazysoundsystem |
Comments
Comment #1
lazysoundsystem CreditAttribution: lazysoundsystem commentedHere's the patch.
Comment #2
lazysoundsystem CreditAttribution: lazysoundsystem commentedChanging to 'needs review'.
Comment #3
lazysoundsystem CreditAttribution: lazysoundsystem commentedI 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.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commented#1: remove-t-from-Contact-asserts-1742830-1.patch queued for re-testing.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedAll of the changes removing t() from assert messages are appropriate. The re-test is green too so RBTC.
Comment #6
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #7
xjmWe're backporting these. :)
Comment #8
andypostThere's a some strings are still there, also I recommend to remove a group (Contact) from asserts. Probably $this->assertEqual(value1, value2) is enough....
this t() should not be there
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedAtached is a patch which addresses the missing t() removals from messages.
@andypost - Can you open up a separate issue to remove the group parameter?
Comment #10
andypost@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
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #12
lazysoundsystem CreditAttribution: lazysoundsystem commentedThanks 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
Comment #13
andypostGreat!
Comment #14
webchickSince this concerns coding standards, moving to Jennifer's queue.
Comment #15
jhodgdonUmmm... why is the group "contact" being *added* to these tests? We shouldn't be adding groups to tests.
Comment #16
jhodgdonComment #17
andypostI think this should wait for #1588422: Convert contact categories to configuration system
Comment #18
dcam CreditAttribution: dcam commentedThe t() functions shown in #12 were removed by http://drupal.org/node/1588422#comment-6587438. Moving to D7 for backport.
Comment #19
dcam CreditAttribution: dcam commentedBackported #1 to D7.
Comment #20
andypost#19: 1742830-19-contact-t.patch queued for re-testing.
Comment #21
izus CreditAttribution: izus commented#19: 1742830-19-contact-t.patch queued for re-testing.
Comment #22
izus CreditAttribution: izus commentedseems good.
Thanks !
Comment #23
jhodgdonThanks all! Committed to 7.x.