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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

FileSize
22.02 KB

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

Lars Toomre’s picture

Status: Active » Needs review

Helps if I give it the right status!

xjm’s picture

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

Lars Toomre’s picture

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

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
22.03 KB

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

jhodgdon’s picture

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.

Lars Toomre’s picture

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?

jhodgdon’s picture

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?

Lars Toomre’s picture

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.

Lars Toomre’s picture

FileSize
3.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.

jhodgdon’s picture

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

jhodgdon’s picture

Status: Needs work » Needs review
Lars Toomre’s picture

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

jhodgdon’s picture

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

Lars Toomre’s picture

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.

jhodgdon’s picture

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

Lars Toomre’s picture

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

dcam’s picture

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.

Lars Toomre’s picture

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

jhodgdon’s picture

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.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
17.47 KB

Backported #12 to D7.

dcam’s picture

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

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

izus’s picture

Issue tags: -Novice, -Needs backport to D7

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

dcam’s picture

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

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

izus’s picture

Status: Needs review » Reviewed & tested by the community

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

jhodgdon’s picture

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.

Albert Volkman’s picture

Albert Volkman’s picture

Issue summary: View changes

Correcting link in summary.