This is a sub-task of #500866: [META] remove t() from assert message focused on the Form 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 Form sub-system tests for the above. There are approximately 119 changes needed (including format_string() changes) spread out over 17 files.
Comment | File | Size | Author |
---|---|---|---|
#16 | 1797318-16-t-assert-form.patch | 42.86 KB | dcam |
#11 | 1797318-11-t-assert-form.patch | 52.77 KB | Lars Toomre |
#8 | 1797318-8-t-assert-form.patch | 52.16 KB | Lars Toomre |
#6 | t-assert-form-1797318-6.patch | 52.56 KB | Lars Toomre |
#6 | interdiff-1797318-3-6.txt | 5.81 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedCorrecting title... Also noting that there needs to be testUserRegistrationRebuild() looked at closely. It appears that someone took out a t() around 'Rebuild' when they should not have.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedIt appears that there are more than a few instances in the tests for this sub-system where the $submit parameter in drupalPost is not wrapped in t(). This should be followed up on as a part of cleaning up this sub-systems tests.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial untested patch for this issue. This patch includes format_string() conversions as well.
Comment #4
xjmThis looks good for the most part. I found two things to fix:
This change is not correct. These assertions have no overridden message; the string in question is part of the test.
These don't actually need to use
format_string()
since there are no variables. (The$
is part of the message string.) I count eight assertions like this in this class.Please provide an interdiff when rerolling the patch. Thanks!
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedMy understanding is that we had to use format_string when we have escaping in a string. If that is wrong, happy to change.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a revised patch that addresses the issues in #4. Also attached is an interdiff (which happens to be my first ever have created so I hope it makes sense).
Comment #7
dcam CreditAttribution: dcam commented#6 no longer applies due to changes to ElementsVerticalTabsTest.php introduced by #1742438: Vertical tabs header and container rendered even when no vertical tabs. It needs a reroll, but I'm not sure if #1742438 should be considered as a blocker to this issue. It had to be moved back to 8.x due to an issue that was created. So there are likely more test changes coming down the pipe.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for looking at this @dcam. Attached is a a re-roll as #7 noted.
It appears that #1742438: Vertical tabs header and container rendered even when no vertical tabs is not yet ready for a commit to D8. Hence, I think that if this patch is reviewed and marked as RTBC, it can proceed. If the other issue gets committed first, this of course can be re-rolled.
I would like to see another 117 calls to t() eliminated. Thanks in advance for taking a look at this.
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedGrr... Helps to set the right status.
Comment #10
dcam CreditAttribution: dcam commentedI tested #8. There were a couple of additional t()'s around assert messages:
FormTest.php, line 601
TriggeringElementTest.php, line 47
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a patch with the additional two changes. Thanks for the review @dcam.
Comment #12
dcam CreditAttribution: dcam commented#11 looks good to me. I haven't found any additional t()'s around assert messages.
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the review @dcam!!
Comment #14
webchickTum te tum....
Comment #15
jhodgdonThanks all! Committed; time to backport.
Comment #16
dcam CreditAttribution: dcam commentedBackported #11 to D7.
Comment #17
dcam CreditAttribution: dcam commented#16: 1797318-16-t-assert-form.patch queued for re-testing.
Comment #18
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #19
izus CreditAttribution: izus commented#16: 1797318-16-t-assert-form.patch queued for re-testing.
Comment #20
izus CreditAttribution: izus commentedHi,
Patch in #16 looks good.
Thanks
Comment #21
jhodgdonPhew! Another one committed to 7.x. Thanks again!
Comment #22.0
(not verified) CreditAttribution: commentedAdded details of change counts from initial patch.