Remove t() from assertion messages in tests for the locale module

Includes lots of format_string() replacements.

An assertion in LocaleJavascriptTranslation was trying to win the Fewest Lines of Code Award™ with multiple conditions, a ternary, and two uses of t() with arguments inside the assertion. I factored it out into something somewhat more sane:

      // Make sure that the proper context was matched.
      $message = strlen($context) > 0 ? 'Context for %source is %context' : 'Context for %source is blank';
      if (isset($source_strings[$str]) && $source_strings[$str] === $context) {
        $this->pass(format_string($message, $args));
      }
      else {
        $this->fail(format_string($message, $args));
      }
    }

Since the string doesn't need to be translated, the rule about using it directly within t() of course does not apply, which is why we can store the message template with placeholders to a variable for readability.

I think this is a sign that I should stop doing these now. :)

I reviewed the patch in #3 and found a couple of things further to clean up. Attached is a revised patch and an interdiff.

This is going to conflict with #1808374: Wrong plural for french in locale tests depending on which gets in first.

Thanks Lars. The changes in #4 look mostly correct as well (though the group message part wasn't part of the original scope). One issue:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePluralFormatTest.phpundefined
@@ -121,12 +121,24 @@ class LocalePluralFormatTest extends WebTestBase {
+          format_string('Computed plural index for %langcode for count %count is %expected', array(
...
+            '%expect' => $expected_plural_index

Bug here. The placeholder is %expected but the array key is %expect.

I also had to think twice about these messages, since they include t()-style placeholders in them, but they should default to displaying the placeholder as part of the message when it's not set in the array of substitutions.

Thanks for the review @xjm. Good catch on '%expect' where it should be '%expected'.

Regarding t() around the group parameter, in one of the sub-issues of this initiative (sorry, I cannot recall which one), @jhodgdon stated that she would welcome any patches that also remove t() from around the group parameter. Hence, since then, where appropriate, I have added that change as well.

I will try to re-roll a patch for the issue you noted in #6.

My limited git skills are failing me. Apparently since #4 was created, an independent commit was made to LocaleUninstallTest.php. Hence, I am not able to resolve what is going wrong with git apply.

Perhaps someone else can re-roll the patch in #4 that includes the comment for #6. Thanks in advance.

@Lars Toomre, there are two things you can try. One would be to reroll the patch with rebase and resolve any merge conflicts. The other would be to apply the patch with git apply --reject, which would apply all the lines but the ones that no longer work, and then re-add those changes manually if appropriate.

Thanks @xjm. The second option in #9 worked and I simply went through and made all of the changes again in LocaleUninstallTest.php. Hence, that file too needs to reviewed carefully again in the attached patch. I am unable to create an interdiff at present.

Grr.. Need to remember to change the status.

Glad to see that worked.

I reviewed #10 carefully both locally and in Dreditor (gotta love the new patch format from .gitattributes) and didn't find any additional errors. (Several things to be careful of in this patch since it's from the module that tests t() itself!)

I can't RTBC this because I created the original patch.

I obviously cannot RBTC this patch either. Perhaps we can recruit someone (beside @jhodgdon) to do a review so that @jhodgdon can do a further review before commit?

I looked though he sources changes, and didn't notice any problems. The basic change involved removing t() from the second argument to assertRaw().

There is the issue of "1 heure" to "@count heure" as noted in a separated issue #1808374.
$this->assertRaw("msgid \"1 hour\"\nmsgid_plural \"@count hours\"\nmsgstr[0] \"1 heure\"\nmsgstr[1] \"@count heures\"", 'Plural translations exported properly.');

So, did a fresh d8 install from git, enabled locale and added the french language.
Ran the locale tests from admin/config/development/testing
Applied the patch in #10, and reran the tests and verified that the results are exactly the same (see attached screenshot).
==> Is this the testing required? Please advise what you'd like done.
Now, I'm pretty new to units tests, so I'm not sure if this review i enough. I presume the idea was to see if the lines where t() was removed contained syntax errors or cause tests to fail?

#10: 1797364-10-t-locale.patch queued for re-testing.

Issue #1809962: Move some locale updates to update.inc for a safe language upgrade. was committed so this will need to re-rolled.

Thanks @boran for the review! Yep, for this issue, it's basically just a matter of ensuring that each change is transparent (i.e. that it only has the effect of removing the coupling to translation, without altering the actual assertion displayed at all or accidentally altering different parameters).

git diff --color-words --unified=0 with the patch applied is a good way to scan the changes, and for trickier bits running the test locally and checking the assertions is a good idea too, though in most cases that's not necessary. Testbot takes care of checking for failed tests and syntax errors for us.

#1812170: Remove dependency on the standard profile from LocaleContentTest will probably collide with this as well.

Rerolled #10. interdiff.txt shows additional t() functions removed after the reroll.

#20: 1797364-20-t-locale.patch queued for re-testing.

hi,
the patch seems good.
Thanks

I was taking a look at committing this, but really I don't think the changes to some of the tests' syntax in several places are very good. They make the code less clear IMO. I'll leave this for someone else to commit if they want to.

I don't really get why we need to use a really complicated format_string() in place of what was a fairly simple string concatenation in a test assertion?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePluralFormatTest.phpundefined
@@ -121,12 +121,24 @@ function testGetPluralFormat() {
-        $this->assertIdentical(locale_get_plural($count, $langcode), $expected_plural_index, 'Computed plural index for ' . $langcode . ' for count ' . $count . ' is ' . $expected_plural_index);
+        $this->assertIdentical(locale_get_plural($count, $langcode), $expected_plural_index,
+          format_string('Computed plural index for %langcode for count %count is %expected', array(
+            '%langcode' => $langcode,
+            '%count' => $count,
+            '%expected' => $expected_plural_index
+          ))
+        );
         // Assert that the we get the right translation for that. Change the
         // expected index as per the logic for translation lookups.
         $expected_plural_index = ($count == 1) ? 0 : $expected_plural_index;
         $expected_plural_string = str_replace('@count', $count, $plural_strings[$langcode][$expected_plural_index]);
-        $this->assertIdentical(format_plural($count, '1 hour', '@count hours', array(), array('langcode' => $langcode)), $expected_plural_string, 'Plural translation of 1 hours / @count hours for count ' . $count . ' in ' . $langcode . ' is ' . $expected_plural_string);
+        $this->assertIdentical(format_plural($count, '1 hour', '@count hours', array(), array('langcode' => $langcode)), $expected_plural_string,
+          format_string('Plural translation of 1 hours / @count hours for count %count in %langcode is %expected', array(
+            '%count' => $count,
+            '%langcode' => $langcode,
+            '%expected' => $expected_plural_string
+          ))
+        );

These changes are out-of-scope... we're not removing a call to t() here.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.phpundefined
@@ -83,7 +83,7 @@ function testUninstallProcess() {
-    $this->assertTrue($result = file_exists($js_file), t('JavaScript file created: %file', array('%file' => $result ? $js_file : t('none'))));
+    $this->assertTrue($result = file_exists($js_file), format_string('JavaScript file created: %file', array('%file' => $result ? $js_file : 'none')));

@@ -109,31 +109,31 @@ function testUninstallProcess() {
-    $this->assertTrue($result = !file_exists($js_file), t('JavaScript file deleted: %file', array('%file' => $result ? $js_file : t('found'))));
+    $this->assertTrue($result = !file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $result ? $js_file : 'found')));

The messages here look bogus... surely we the creation of deletion is unsuccessful we want to know the name of the file!... the following looks much simpler to me...

  $this->assertTrue(file_exists($js_file), format_string('JavaScript file created: %file', array('%file' => $js_file)));
...
  $this->assertFalse(file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $js_file)));

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.phpundefined
@@ -90,12 +90,18 @@ function testFileParsing() {
       // Make sure that the proper context was matched.
-      $this->assertTrue(isset($source_strings[$str]) && $source_strings[$str] === $context, strlen($context) > 0 ? t("Context for %source is %context", $args) : t("Context for %source is blank", $args));
+      $message = strlen($context) > 0 ? 'Context for %source is %context' : 'Context for %source is blank';
+      if (isset($source_strings[$str]) && $source_strings[$str] === $context) {
+        $this->pass(format_string($message, $args));
+      }
+      else {
+        $this->fail(format_string($message, $args));
+      }

This change is not logically equivalent... something like the following is..

      $test = isset($source_strings[$str]) && $source_strings[$str] === $context;
      if (strlen($context) > 0) {
        $this->assertTrue($test, format_string('Context for %source is %context', $args));
      }
      else {
        $this->assertTrue($test, format_string('Context for %source is blank', $args));
      }

Bump! There are only a few of these "remove t()" issues left. Does someone want to make a new patch addressing the comments in #24 so we can get this one done?

I'm camping this weekend. I probably won't be able to fix it until Tuesday. Maybe someone else will get to it first. It would be nice to finally get these reviewed and out of my issue queue.

Rerolled #20, then made the changes from #24. Changes are shown in interdiff.txt.

At last I think we're going to get to the end of this issue... RTBC.

Sorry, but the patch does not apply today.

rerolled patch in the tests for the locale module.

rerolled patch in the tests for the locale module.

The cmi statistics patch in #31 involves quite a few other changes. Re-rolling the patch in #27 has one change that needed to be removed, and removal of another remaining t().

These from the patch in #31

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -68,7 +68,11 @@ function testUninstallProcess() {
+<<<<<<< HEAD
     $this->assertEqual(language(Language::TYPE_INTERFACE)->id, $this->langcode, t('Current language: %lang', array('%lang' => language(Language::TYPE_INTERFACE)->id)));
+=======

This change should probably go in a different issue.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -115,31 +119,35 @@ function testUninstallProcess() {
+<<<<<<< HEAD
     $this->assertEqual(language(Language::TYPE_INTERFACE)->id, 'en', t('Language after uninstall: %lang', array('%lang' => language(Language::TYPE_INTERFACE)->id)));
+=======
+    $this->assertEqual(language(Language::TYPE_INTERFACE)->langcode, 'en', format_string('Language after uninstall: %lang', array('%lang' => language(Language::TYPE_INTERFACE)->langcode)));
+>>>>>>> Rerollong the patch for the locale module https://drupal.org/node/1797364#comment-7573647
 
     // Check JavaScript files deletion.
-    $this->assertTrue($result = !file_exists($js_file), t('JavaScript file deleted: %file', array('%file' => $result ? $js_file : t('found'))));
+    $this->assertFalse(file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $js_file)));

and this too.

And the many changes to LocaleUpdateTest.php

Sorry for the noise, there was another change, from '->langcode' to '->id'. This one applies.

THIS IS THE LAST ONE OF THESE plain-vanilla remove t() from test assert issues!!!! (at least, I think it is)... Can someone please review it so we can get it into 8.x, then backport to 7.x, and be DONE with the meta-issue? :)

I do not have a local D8 installation to apply the patch in #35 against. Hence, I am unable to confirm that the resulting patched file has caught all of the appropriate t() calls.

However, what is changed in the patch I can confirm is all correct. Hence, I think that this final pain-vanilla remove t() patches is ready to go!

There are still problems with this patch:

a) This segment of LocaleJavascriptTranslation.php is not quite right:

      // Make sure that the proper context was matched.
-      $this->assertTrue(isset($source_strings[$str]) && $source_strings[$str] === $context, strlen($context) > 0 ? t("Context for %source is %context", $args) : t("Context for %source is blank", $args));
+     $test = isset($source_strings[$str]) && $source_strings[$str] === $context;
+      if (strlen($context) > 0) {
+        $this->assertTrue($test, format_string('Context for %source is %context', $args));
+      }
+      else {
+        $this->assertTrue($test, format_string('Context for %source is blank', $args));
+      }

For one thing, the indentation of the first line is wrong. For another thing, let's just keep the code as it was and remove t() rather than making completely new code. Out of scope for this issue. Thanks!

b) Also, on another issue we were asked to stop using format_string():
#2035077-2: removing t() from asserts - remaining changes.
so I guess I cannot commit this until that is addressed for lines changed in this patch. :(

c) Don't change the code around in tests -- out of scope -- such as this segment of LocaleUninstallTest.php:

-    $this->assertTrue($result = !file_exists($js_file), t('JavaScript file deleted: %file', array('%file' => $result ? $js_file : t('found'))));
+    $this->assertFalse(file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $js_file)));

Here's a reroll and the changes from #38.

Let's try this again. A '}' was accidentally deleted.

Thanks @dcam for taking this back to basics, it had started to get out of control. The patch applies fine and looks good, but it needs use Drupal\Component\Utility\String; at the top of each file that uses String::format.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php
@@ -96,12 +96,12 @@ function testFileParsing() {
+      $this->assertTrue(isset($source_strings[$str]), String::format("Found source string: %source", $args));

here,

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationUiTest.php
@@ -261,14 +261,14 @@ function testJavaScriptTranslation() {
+    $this->assertTrue($result = file_exists($js_file), String::format('JavaScript file created: %file', array('%file' => $result ? $js_file : 'not found')));

and here,

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -89,7 +89,7 @@ function testUninstallProcess() {
+    $this->assertTrue(file_exists($js_file), String::format('JavaScript file created: %file', array('%file' => $js_file)));

and here.

Oops. I guess I expected the namespace was already loaded or maybe I just wasn't thinking.

Thanks - this is now complete. RTBC.

Ummm... This change in LocaleUninstallTest still doesn't look right to me:

-    $this->assertTrue($result = file_exists($js_file), t('JavaScript file created: %file', array('%file' => $result ? $js_file : t('none'))));
+    $this->assertTrue(file_exists($js_file), String::format('JavaScript file created: %file', array('%file' => $js_file)));

I think the message used to say "none" if the test failed and the file did not exist. That won't happen with the patch.

Everything else looks OK.

I checked for other problems, but obviously wasn't diligent enough. I also made certain to check for newer t() functions and found one.

Well, this issue has already been RTBC four times (twice by me), so I'm a bit hesitant to try once more. RTBC.

Needs a reroll...

git ac https://drupal.org/files/1797364-47-t-locale.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 51995  100 51995    0     0  15942      0  0:00:03  0:00:03 --:--:-- 27409
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php:125
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php: patch does not apply
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.php:110
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.php: patch does not apply
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php:112
error: core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php: patch does not apply
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php:89
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php: patch does not apply

Ugh.... rerolled #47.

For what it's worth, once more: RTBC.

THANKS! Committed to 8.x (before it needs another reroll). :)

7.x should require much less rerolling and anguish... anyone up for a backport?

Oh, thank goodness... I wasn't looking forward to the possibility of rerolling it yet again for D8. I'll backport it later unless someone beats me to it.

Backport of #50.

+++ b/modules/locale/locale.test
@@ -1492,44 +1492,44 @@ class LocaleUninstallFunctionalTest extends DrupalWebTestCase {
-    $this->assertTrue($language_negotiation, t('Interface language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
+    $this->assertTrue($language_negotiation, format_string('Interface language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
     $language_negotiation = language_negotiation_get(LANGUAGE_TYPE_CONTENT) == LANGUAGE_NEGOTIATION_DEFAULT;
-    $this->assertTrue($language_negotiation, t('Content language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
+    $this->assertTrue($language_negotiation, format_string('Content language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
     $language_negotiation = language_negotiation_get(LANGUAGE_TYPE_URL) == LANGUAGE_NEGOTIATION_DEFAULT;
-    $this->assertTrue($language_negotiation, t('URL language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
+    $this->assertTrue($language_negotiation, format_string('URL language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));

This all looks good except for an inconsistency with t()s inside 'format_string()' calls. All have been removed, except these three.

Here's #54 + the changes from #55.

Thanks, RTBC.

-    $this->drupalPost(NULL, $edit, t('Save translations'));
...
+    $this->drupalPost(NULL, $edit, 'Save translations');

I'm not sure that one is right, but there's no way I'm holding up what is apparently the last one of these "remove t()" issues over that!!! (I think it doesn't matter much in practice, and we could always have a followup to add it back.)

Therefore, committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e516351

If this is really the last one of these issues, someone should throw a very large party. Great work everyone.

This 'Save translations' looks wrong to me as it fails in other site languages.

Yes, that one was wrong. (#58). Follow-up please?

Here's the follow-up.

Looks good - thanks!

FYI, the reason I don't think this matters much in practice is that this test (like almost all tests) runs in English. It doesn't matter if the parent site is in another language; this code runs against the site that is being tested so it should pass regardless.

But it still makes sense to fix this anyway, for consistency etc.

@jhodgdon, you can certainly feel free to commit this whenever you want (including right before the 7.23 release - this patch is definitely not subject to "code freeze").

Thanks! Follow-up committed to 7.x

And, this was the *****LAST ONE*****

(cheers!)