This is a sub-task of #500866: [META] remove t() from assert message focused on the G-L include files from the system module sub-systems.
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 Graph, Image and Lock sub-system tests for the above. There are approximately 59 changes needed spread out across 5 test files. This issue includes conversion of all format_patch() occurances as well.
Comment | File | Size | Author |
---|---|---|---|
#22 | t-assert-G-L-1797410-22.patch | 17.47 KB | dcam |
#12 | t-assert-G-L-1797410-9.patch | 22 KB | jhodgdon |
#11 | interdiff-1797410-1-9.txt | 3.68 KB | Lars Toomre |
#9 | t-assert-G-L-1797410-9.patch | 22 KB | Lars Toomre |
#5 | t-assert-G-L-1797410-5.patch | 22.03 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial untested patch for this issue. This patch includes format_string() conversions as well.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedHelps if I give it the right status!
Comment #3
xjmPhew, a smaller patch. One thing here:
These changes (and subsequent in this class) are not correct. The third parameter here is the group name, and should be translated. The message text is the second parameter. See: TestBase::assertTrue().
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedOh, I did screw that up. Good catch! I will re-roll in the morning when I am at my development machine.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an updated patch and interdiff addressing the issues in #3.
Comment #6
jhodgdonI am not sure about this change in #5 -- see discussion on #1797120-15: Remove t() from asserts for A-C includes system tests and the next few comments after that. Let's discuss it there until we resolve the question, or else file a separate issue and resolve it there.
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedThe changes in #5 are re-apply t() to the group parameters and correctly removing it from message parameters. Hence, it acheives what this issue is focused on.
I agree that we should have a follow up issue to remove t() from around groups and probably address what the heck groups are for. Maybe the solution is to add more group parameters to many of the assertions?
Comment #8
jhodgdonAs per the discussion on that other issue, let's leave the t() off the $group parameters (it shouldn't be there) and just follow up with removing it off the $message parameters (which is what this issue is about). Thanks!
And no the solution is *not* to add more $group parameters. That is only to override the group default set up in getInfo() and there is IMO never really a need for that I think?
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an updated patch that removes t() from around all of those 'Lock' group parameters as well. This now includes removal of t() from both message and group in that one lock test. Everything well is the same as patch in #1.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedJust checked the interdiff between #1 and #9 (attached). It sure does not look like this failure is due to something with this patch.
Comment #12
jhodgdonNo, that failure is in EntityTranslationTest. Hm. Hopefully it wasn't due to one of the other test cleanups today...
Here's the patch uploaded again so we can run the tests again without wiping out the other test result.
DO NOT GIVE ME CREDIT FOR THIS PATCH. :)
Comment #13
jhodgdonComment #14
Lars Toomre CreditAttribution: Lars Toomre commented@jhodgdon by chance EntityTranslationTest is what was giving me trouble in #1797220: Remove t() from asserts from Entity sub-system tests. There was an external change to that test that was causing a problem getting that patch to re-apply to my local git repository. So I am pretty sure this is not due to anything we have done.
Comment #15
jhodgdonWell, we probably need to find the commit that caused it and revert it...
Comment #16
Lars Toomre CreditAttribution: Lars Toomre commentedAll the requested changes are in #12 and it passes. If it was not my own patch, I would RTBC this.
However, I do not know what further to do about failure in #9.
Comment #17
jhodgdonberdir filed an issue with that test failure from #9 (not due to this patch):
#1798382: Random Test failures in Entity translation tests
Comment #18
Lars Toomre CreditAttribution: Lars Toomre commented#12: t-assert-G-L-1797410-9.patch queued for re-testing.
Comment #19
dcam CreditAttribution: dcam commented#12 looks good to me. I didn't find any more t()'s around assert messages. Marking as RTBC.
Comment #20
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @dcam for the review. I just added the two additional t() found in the File sub-issue to a new patch there.
Comment #21
jhodgdonThanks! This one is committed to 8.x. I guess it is ready for port.
Comment #22
dcam CreditAttribution: dcam commentedBackported #12 to D7.
Comment #23
dcam CreditAttribution: dcam commented#22: t-assert-G-L-1797410-22.patch queued for re-testing.
Comment #24
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #25
izus CreditAttribution: izus commented#22: t-assert-G-L-1797410-22.patch queued for re-testing.
Comment #27
dcam CreditAttribution: dcam commented#22: t-assert-G-L-1797410-22.patch queued for re-testing.
Comment #28
izus CreditAttribution: izus commentedHi,
The patch in #22 seems ok for me.
Thanks
Comment #29
jhodgdonThanks! Committed to 7.x.
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commentedBacklinking #1579810-18: GD tests do not test check every case and code cleanup
Comment #31.0
Albert Volkman CreditAttribution: Albert Volkman commentedCorrecting link in summary.