This is a sub-task of #500866: [META] remove t() from assert message focused on the A-C 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 Ajax, Batch, Bootstrap, Bundle, Cache, and Common sub-system tests for the above. There are approximately 400 changes needed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
168.29 KB

Here is an initial untested patch for this issue.

Status: Needs review » Needs work

The last submitted patch, t-assert-1797120-1.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
168.25 KB

Darn stray character from editing manually... This patch is also untested locally.

Approximately 40 test files were touched by this patch. Let's see what the bot thinks.

xjm’s picture

Green, nice. I'll take a shot at reviewing it.

Lars Toomre’s picture

Oh other thing to point out @xjm, I corrected some text strings to use single quotes rather than double quotes where there was no single quote in the message text. I think that is permissible as I go through reading each of the tests one by one. *smiles*

xjm’s picture

Sure, I think that's fine so long as we don't require it reviewing other issues.

Also note #1681890: Use format_string() instead of t() for assertion message text. In this case I think it's fine since you've already rolled the patch, but for additional ones, we can split it into two smaller patches.

Lars Toomre’s picture

I have been handling them together as I have had to review each assertion message. If there are only a few, I would prefer to catch them rather having to go back through all of the tests looking for just that issue. Fair enough?

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Yep, that works for me. I added an asterisk to the issues in the meta that have format_string() covered already.

And, I reviewed this whole patch. I think I sort of went into a trance, but I didn't find a single error. Nice work!

xjm’s picture

Assigned: Unassigned » jhodgdon

Oh, assigning to @jhodgdon since it's a cleanup patch.

Lars Toomre’s picture

Assigned: jhodgdon » Unassigned

Thank you... That was done as I watched Monday night's NFL football game. Great way to pass time during commercials; reviewing assert messages!! LOL

Lars Toomre’s picture

Whoops... cross-post! Not sure how to correct the assignment.

xjm’s picture

Assigned: Unassigned » jhodgdon

Fixed. :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

As I was paging through the diff, I found one more t() that needs to be fixed in
core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
and there may be more...

Also, this set of changes caught the $group parameter (why that is even there I don't know???) but not the actual assertion messages:

--- a/core/modules/system/lib/Drupal/system/Tests/Common/ParseInfoFileUnitTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/ParseInfoFileUnitTest.php
-    $this->assertEqual($info_values['simple_string'], 'A simple string', t('Simple string value was parsed correctly.'), t('System'));
-    $this->assertEqual($info_values['simple_constant'], WATCHDOG_INFO, t('Constant value was parsed correctly.'), t('System'));
-    $this->assertEqual($info_values['double_colon'], 'dummyClassName::', t('Value containing double-colon was parsed correctly.'), t('System'));
+    $this->assertEqual($info_values['simple_string'], 'A simple string', t('Simple string value was parsed correctly.'), 'System');
+    $this->assertEqual($info_values['simple_constant'], WATCHDOG_INFO, t('Constant value was parsed correctly.'), 'System');
+    $this->assertEqual($info_values['double_colon'], 'dummyClassName::', t('Value containing double-colon was parsed correctly.'), 'System');

But I didn't see any problems with the cleanups in the patch, so I committed it and am now setting back to "needs work" for follow-ups.

jhodgdon’s picture

forgot to unassign

xjm’s picture

Whoops, yeah, the hunk in #13 should be reverted. I caught that in one of the other patches, but not this one. The group strings technically should still have t().

jhodgdon’s picture

Really? I don't think the group strings should have t() in them. The objective of not having t() in test assertions is that tests are run in English by developers. The groups are just as stupid to translate as the messages, right?

Lars Toomre’s picture

I orginally thought that group parameters should be taken out, but now am confused. I guess I really do not understand how the group parameters are used, especially since they are used in a small portion of all tests.

I just posted an update to #1797410-5: Remove t() from asserts for G-L includes system tests that put them back in.

jhodgdon’s picture

Yeah, I have no idea why all the test assertion functions have a group parameter, and I have no idea why we are using them or what for, and I definitely don't think they should be translated if we are using them... I'll cross-reference this on the other issue but we should decide before we go around re-translating them...

jhodgdon’s picture

OK, I talked briefly about this with xjm in IRC just now. We have agreed (hopefully others will too):
- The group names are not translated (or shouldn't be) in getInfo() so they shouldn't be translated when passing a string into an assertion for $group either.
- However, removing t() from the $group is out of scope for this meta-issue.
- Also, we probably don't need to pass $group into any test assertions in Core.
- However, removing $group parameters to test assertions entirely is also out of scope for this meta-issue.
- That said, if t() was removed by mistake from some $group parameters in some of the patches, let's not follow-up by putting it back in.

So... in summary: let's leave the commit above as-is, but fix the assertions mentioned in #13.

Lars Toomre’s picture

May I recommend as we encounter t() in getInfo() or t() around group that we make those changs? Quite frankly, I shudder at the thought of manually examing so many test files again. From looking at so many tests, there are not many of them (<20% I would guess as a max). Fair enough?

jhodgdon’s picture

I will be happy to commit patches that remove t() from $group parameters.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

really unassigning this time.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
9.45 KB

Attached is a follow up patch that catches the last couple of t() asserts as well as what @jhodgdon noted in #13.

Note: I checked the all of the tests in A-C Tests with grep now that the bulk of the changes were applied. I am pretty sure that this now catches all of the t() around message asserts. Also did not find any t() in any getInfo() parameters in this group.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Component: ajax system » base system
Status: Needs review » Patch (to be ported)

I committed this small patch too, and I guess it's ready for backport now.

Backport note: When this issue is backported to D7, it should include things testing actions.inc that were included in a couple of other 8.x issues (things that were moved to modules in d8):
#1798954: Remove t() from test asserts in Action module [part of system tests in d7]
#1798960: Remove t() from test asserts in Ban module [system module include in D7]

Also, both the patch in #23 and the earlier patch in #3 need to be included.

dcam’s picture

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

Backported #3 and #23 to D7.

Status: Needs review » Needs work

The last submitted patch, system-asserts-1797120-25.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review

#25: system-asserts-1797120-25.patch queued for re-testing.

dcam’s picture

#25: system-asserts-1797120-25.patch queued for re-testing.

dcam’s picture

#25: system-asserts-1797120-25.patch queued for re-testing.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

jhodgdon’s picture

Issue tags: -Novice

#25: system-asserts-1797120-25.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, system-asserts-1797120-25.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

dcam’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
134.48 KB

Rerolled #25.

jhodgdon’s picture

bump! We only have a couple of these left. Can someone review this patch?

lazysoundsystem’s picture

Status: Needs review » Needs work

Sorry, didn't see this was waiting - it's very nearly there, just three remaining:

+++ b/modules/simpletest/tests/common.test
@@ -2188,9 +2188,9 @@ class ParseInfoFilesTestCase extends DrupalUnitTestCase {
-    $this->assertEqual($info_values['simple_string'], 'A simple string', t('Simple string value was parsed correctly.'), t('System'));
-    $this->assertEqual($info_values['simple_constant'], WATCHDOG_INFO, t('Constant value was parsed correctly.'), t('System'));
-    $this->assertEqual($info_values['double_colon'], 'dummyClassName::', t('Value containing double-colon was parsed correctly.'), t('System'));
+    $this->assertEqual($info_values['simple_string'], 'A simple string', t('Simple string value was parsed correctly.'), 'System');
+    $this->assertEqual($info_values['simple_constant'], WATCHDOG_INFO, t('Constant value was parsed correctly.'), 'System');

The t() has been removed from the group, but not the message.

dcam’s picture

Fixed the asserts shown in #37.

dcam’s picture

Status: Needs work » Needs review

Status...

lazysoundsystem’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, those three from #37 are now fixed. RTBC.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all! Committed to 7.x.

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