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

Title:Remove t() from asserts from File sub-system testsRemove 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.

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.

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.

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!

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

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

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.

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.

Status:Needs work» Needs review

Grr... Helps to set the right status.

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

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.

Status:Needs review» Reviewed & tested by the community

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

Thanks for the review @dcam!!

Assigned:Lars Toomre» jhodgdon

Tum te tum....

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.

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

Backported #11 to D7.

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

Issue tags:+Novice

Tagging as Novice.

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

Status:Needs review» Reviewed & tested by the community

Hi,
Patch in #16 looks good.
Thanks

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.

Issue summary:View changes

Added details of change counts from initial patch.