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.

Files: 
CommentFileSizeAuthor
#38 1797120-38-system-asserts.patch134.47 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,336 pass(es).
[ View ]
#35 1797120-35-system-asserts.patch134.48 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,173 pass(es).
[ View ]
#25 system-asserts-1797120-25.patch133.76 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-asserts-1797120-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 1797120-23-t-assert-A-C.patch9.45 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,897 pass(es).
[ View ]
#3 t-assert-1797120-3.patch168.25 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,698 pass(es).
[ View ]
#1 t-assert-1797120-1.patch168.29 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Common/HttpRequestTest.php.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new168.29 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Common/HttpRequestTest.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new168.25 KB
PASSED: [[SimpleTest]]: [MySQL] 41,698 pass(es).
[ View ]

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.

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

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*

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.

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?

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!

Assigned:Unassigned» jhodgdon

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

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

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

Assigned:Unassigned» jhodgdon

Fixed. :)

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.

forgot to unassign

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

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?

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.

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

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.

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?

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

Assigned:jhodgdon» Unassigned

really unassigning this time.

Status:Needs work» Needs review
StatusFileSize
new9.45 KB
PASSED: [[SimpleTest]]: [MySQL] 41,897 pass(es).
[ View ]

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.

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new133.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-asserts-1797120-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Backported #3 and #23 to D7.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

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

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

Issue tags:+Novice

Tagging as Novice.

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.

Issue tags:+Needs reroll

Tagging for reroll.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new134.48 KB
PASSED: [[SimpleTest]]: [MySQL] 40,173 pass(es).
[ View ]

Rerolled #25.

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

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.

StatusFileSize
new134.47 KB
PASSED: [[SimpleTest]]: [MySQL] 40,336 pass(es).
[ View ]

Fixed the asserts shown in #37.

Status:Needs work» Needs review

Status...

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

Thanks all! Committed to 7.x.

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