This is a sub-task of #500866: [META] remove t() from assert message focused on the Form sub-system.

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 Form sub-system tests for the above. There are approximately 119 changes needed (including format_string() changes) spread out over 17 files.

Files: 
CommentFileSizeAuthor
#16 1797318-16-t-assert-form.patch42.86 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,331 pass(es).
[ View ]
#11 1797318-11-t-assert-form.patch52.77 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 42,812 pass(es).
[ View ]
#8 1797318-8-t-assert-form.patch52.16 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 42,800 pass(es).
[ View ]
#6 t-assert-form-1797318-6.patch52.56 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]
#6 interdiff-1797318-3-6.txt5.81 KBLars Toomre
#3 t-assert-form-1797318-3.patch53.67 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,698 pass(es).
[ View ]

Comments

Lars Toomre’s picture

Title:Remove t() from asserts from File sub-system tests» Remove t() from asserts from Form sub-system tests
Assigned:Unassigned» Lars Toomre

Correcting title... Also noting that there needs to be testUserRegistrationRebuild() looked at closely. It appears that someone took out a t() around 'Rebuild' when they should not have.

Lars Toomre’s picture

It appears that there are more than a few instances in the tests for this sub-system where the $submit parameter in drupalPost is not wrapped in t(). This should be followed up on as a part of cleaning up this sub-systems tests.

Lars Toomre’s picture

Status:Active» Needs review
StatusFileSize
new53.67 KB
PASSED: [[SimpleTest]]: [MySQL] 41,698 pass(es).
[ View ]

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

xjm’s picture

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

This looks good for the most part. I found two things to fix:

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/EmailTest.phpundefined
    @@ -39,8 +39,8 @@ class EmailTest extends WebTestBase {
    -    $this->assertRaw(t('The e-mail address %mail is not valid.', array('%mail' => 'invalid')));
    -    $this->assertRaw(t('!name field is required.', array('!name' => 'Address')));
    +    $this->assertRaw(format_string('The e-mail address %mail is not valid.', array('%mail' => 'invalid')));
    +    $this->assertRaw(format_string('!name field is required.', array('!name' => 'Address')));

    This change is not correct. These assertions have no overridden message; the string in question is part of the test.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/TriggeringElementTest.phpundefined
    @@ -43,7 +43,7 @@ class TriggeringElementTest extends WebTestBase {
    -    $this->assertText('There is no clicked button.', t('$form_state[\'triggering_element\'] set to NULL.'));
    +    $this->assertText('There is no clicked button.', format_string('$form_state[\'triggering_element\'] set to NULL.'));

    These don't actually need to use format_string() since there are no variables. (The $ is part of the message string.) I count eight assertions like this in this class.

Please provide an interdiff when rerolling the patch. Thanks!

Lars Toomre’s picture

My understanding is that we had to use format_string when we have escaping in a string. If that is wrong, happy to change.

Lars Toomre’s picture

Status:Needs work» Needs review
StatusFileSize
new5.81 KB
new52.56 KB
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]

Here is a revised patch that addresses the issues in #4. Also attached is an interdiff (which happens to be my first ever have created so I hope it makes sense).

dcam’s picture

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

#6 no longer applies due to changes to ElementsVerticalTabsTest.php introduced by #1742438: Vertical tabs header and container rendered even when no vertical tabs. It needs a reroll, but I'm not sure if #1742438 should be considered as a blocker to this issue. It had to be moved back to 8.x due to an issue that was created. So there are likely more test changes coming down the pipe.

Lars Toomre’s picture

Issue tags:-Needs reroll
StatusFileSize
new52.16 KB
PASSED: [[SimpleTest]]: [MySQL] 42,800 pass(es).
[ View ]

Thanks for looking at this @dcam. Attached is a a re-roll as #7 noted.

It appears that #1742438: Vertical tabs header and container rendered even when no vertical tabs is not yet ready for a commit to D8. Hence, I think that if this patch is reviewed and marked as RTBC, it can proceed. If the other issue gets committed first, this of course can be re-rolled.

I would like to see another 117 calls to t() eliminated. Thanks in advance for taking a look at this.

Lars Toomre’s picture

Status:Needs work» Needs review

Grr... Helps to set the right status.

dcam’s picture

Status:Needs review» Needs work

I tested #8. There were a couple of additional t()'s around assert messages:
FormTest.php, line 601
TriggeringElementTest.php, line 47

Lars Toomre’s picture

Status:Needs work» Needs review
StatusFileSize
new52.77 KB
PASSED: [[SimpleTest]]: [MySQL] 42,812 pass(es).
[ View ]

Here is a patch with the additional two changes. Thanks for the review @dcam.

dcam’s picture

Status:Needs review» Reviewed & tested by the community

#11 looks good to me. I haven't found any additional t()'s around assert messages.

Lars Toomre’s picture

Thanks for the review @dcam!!

webchick’s picture

Assigned:Lars Toomre» jhodgdon

Tum te tum....

jhodgdon’s picture

Version:8.x-dev» 7.x-dev
Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks all! Committed; time to backport.

dcam’s picture

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

Backported #11 to D7.

dcam’s picture

#16: 1797318-16-t-assert-form.patch queued for re-testing.

dcam’s picture

Issue tags:+Novice

Tagging as Novice.

izus’s picture

#16: 1797318-16-t-assert-form.patch queued for re-testing.

izus’s picture

Status:Needs review» Reviewed & tested by the community

Hi,
Patch in #16 looks good.
Thanks

jhodgdon’s picture

Status:Reviewed & tested by the community» Fixed

Phew! Another one committed to 7.x. Thanks again!

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

Anonymous’s picture

Issue summary:View changes

Added details of change counts from initial patch.