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.

Files: 
CommentFileSizeAuthor
#22 t-assert-G-L-1797410-22.patch17.47 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,341 pass(es).
[ View ]
#12 t-assert-G-L-1797410-9.patch22 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 42,101 pass(es).
[ View ]
#11 interdiff-1797410-1-9.txt3.68 KBLars Toomre
#9 t-assert-G-L-1797410-9.patch22 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,895 pass(es).
[ View ]
#5 t-assert-G-L-1797410-5.patch22.03 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,890 pass(es).
[ View ]
#5 interdiff-1797410-1-5.txt3.71 KBLars Toomre
#1 t-assert-G-L-1797410-1.patch22.02 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,701 pass(es).
[ View ]

Comments

StatusFileSize
new22.02 KB
PASSED: [[SimpleTest]]: [MySQL] 41,701 pass(es).
[ View ]

Here is an initial untested patch for this issue. This patch includes format_string() conversions as well.

Status:Active» Needs review

Helps if I give it the right status!

Status:Needs review» Needs work
Issue tags:+needs backport to D7

Phew, a smaller patch. One thing here:

+++ b/core/modules/system/lib/Drupal/system/Tests/Lock/LockFunctionalTest.phpundefined
@@ -83,34 +83,34 @@ class LockFunctionalTest extends WebTestBase {
-    $this->assertTrue(lock()->acquire('system_test_lock_acquire'), t('Lock acquired by this request.'), t('Lock'));
-    $this->assertTrue(lock()->acquire('system_test_lock_acquire'), t('Lock extended by this request.'), t('Lock'));
+    $this->assertTrue(lock()->acquire('system_test_lock_acquire'), t('Lock acquired by this request.'), 'Lock');
+    $this->assertTrue(lock()->acquire('system_test_lock_acquire'), t('Lock extended by this request.'), 'Lock');

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().

Oh, I did screw that up. Good catch! I will re-roll in the morning when I am at my development machine.

Status:Needs work» Needs review
StatusFileSize
new3.71 KB
new22.03 KB
PASSED: [[SimpleTest]]: [MySQL] 41,890 pass(es).
[ View ]

Here is an updated patch and interdiff addressing the issues in #3.

I 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.

The 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?

As 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?

StatusFileSize
new22 KB
PASSED: [[SimpleTest]]: [MySQL] 41,895 pass(es).
[ View ]

Here 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.

Status:Needs review» Needs work

The last submitted patch, t-assert-G-L-1797410-9.patch, failed testing.

StatusFileSize
new3.68 KB

Just checked the interdiff between #1 and #9 (attached). It sure does not look like this failure is due to something with this patch.

StatusFileSize
new22 KB
PASSED: [[SimpleTest]]: [MySQL] 42,101 pass(es).
[ View ]

No, 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. :)

Status:Needs work» Needs review

@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.

Well, we probably need to find the commit that caused it and revert it...

All 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.

berdir filed an issue with that test failure from #9 (not due to this patch):
#1798382: Random Test failures in Entity translation tests

#12: t-assert-G-L-1797410-9.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

#12 looks good to me. I didn't find any more t()'s around assert messages. Marking as RTBC.

Thanks @dcam for the review. I just added the two additional t() found in the File sub-issue to a new patch there.

Version:8.x-dev» 7.x-dev
Component:image system» system.module
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks! This one is committed to 8.x. I guess it is ready for port.

Status:Patch (to be ported)» Needs review
StatusFileSize
new17.47 KB
PASSED: [[SimpleTest]]: [MySQL] 40,341 pass(es).
[ View ]

Backported #12 to D7.

#22: t-assert-G-L-1797410-22.patch queued for re-testing.

Issue tags:+Novice

Tagging as Novice.

#22: t-assert-G-L-1797410-22.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, t-assert-G-L-1797410-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7

#22: t-assert-G-L-1797410-22.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Hi,
The patch in #22 seems ok for me.
Thanks

Status:Reviewed & tested by the community» Fixed

Thanks! Committed to 7.x.

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Correcting link in summary.