Part of #500866: [META] remove t() from assert message

The TestBase class should not contain calls to t() for format_plural().

All t() calls should be replaced with String::format().

Plain text strings without token replacements can be output directly.

Calls to format_plural() are to be replaced with English plural rules, which means this:

- $message = format_plural($emailCount, '1 e-mail was sent during this test.', '@count e-mails were sent during this test.');
+ $message = $emailCount == 1 ? '1 e-mail was sent during this test.' : $emailCount . ' e-mails were sent during this test.';

Original summary:

As part of #500866: [META] remove t() from assert message, I noticed that the base test classes also have t() included in the creation of their default test assertion messages. For instance:

  protected function assertTrue($value, $message = '', $group = 'Other') {
    return $this->assert((bool) $value, $message ? $message : t('Value @value is TRUE.', array('@value' => var_export($value, TRUE))), $group);
  }

(this is in TestBase)

These t() calls need to be replaced with format_string(). I decided it was separate from
#1797514: Remove t() from assertion messages in tests for the simpletest module
which is only addressing the tests.

CommentFileSizeAuthor
#14 interdiff.txt750 bytessun
#12 drupal8.testbase-t.12.patch17.97 KBsun
#9 drupal8.testbase-t.9.patch17.96 KBsun
#7 drupal8.testbase-t.7.patch15.68 KBsun
#2 1803674-2-t-testbase.patch30.25 KBlars toomre

Comments

lars toomre’s picture

lars toomre commented

I am starting to work on this. In doing so, I notice that there are a number of documentation issues that need to be addressed in TestBase.php (like missing @param directives). These files do not look they were covered by #1431636: Clean up API docs for simpletests module (excluding files subdirectory).

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new 1803674-2-t-testbase.patch30.25 KB

Here is an untested patch for this issue. It includes conversion of t() to format_string() as appropriate. It also includes a few removals of t() from static strings and removal of t() from appropriate group parameters.

It will take a bit of care to review since there are several cases of $message variables being changed before it is used below in an assert.

Let's see what the bot thinks.

lars toomre’s picture

lars toomre commented

I opened up #1805264: Clean up API docs for Test base classes in Simpletest module to address cleaning up the docblocks in these three files. I will file a patch there later this weekend.

EDIT: I now have added the patch to #1805264-1: Clean up API docs for Test base classes in Simpletest module and it has come back green ready for review.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Related issue
#1813286: _simpletest_format_summary_line() should not use t()

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Closed (duplicate)

This is apparently a duplicate of #1601146: Allow custom assertion messages using predefined placeholders, which has a more involved patch on it.

olli’s picture

olli commented
Issue summary: View changes
Status: Closed (duplicate) » Needs review

per #1601146-33: Allow custom assertion messages using predefined placeholders

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: Unassigned » sun
Issue tags: +Testing system
StatusFileSize
new drupal8.testbase-t.7.patch15.68 KB

Picking this up again. Attached patch replaces all remaining instances of t(), format_string(), and format_plural() in TestBase.

I'm intentionally not touching the @todo in TestBase::prepareEnvironment() about these remaining uses, since cleaning that up is a separate task.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Remove t() from default test assertions in test base classe » Remove t() from default test assertions in test base classes
Status: Needs review » Needs work

This looks like a straightforward change and should be done.

The only problem is these changes:

-   *   messages: use format_string() to embed variables in the message text, not
+   *   messages: use String::format() to embed variables in the message text, not

They make the docs line go over 80 characters, so the word "not" should be wrapped to the next line. Or just don't make that change -- the format_string() function is not deprecated so there really isn't an ultra-strong reason not to leave that documentation alone?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Remove t() from default test assertions in test base classes » Remove t() from default test assertions in TestBase class
Status: Needs work » Needs review
StatusFileSize
new drupal8.testbase-t.9.patch17.96 KB

Adjusted the phpDoc of all instances as requested.

format_string() is expected to be removed from core very soon.

The last submitted patch, 7: drupal8.testbase-t.7.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Excellent, thanks! Assuming test bot agrees, this should be good. I verified that the changes are all correct on the last go-around. An interdiff might have been nice. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.testbase-t.12.patch17.97 KB 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1143,8 +1157,8 @@ private function restoreEnvironment() {
+        $message = $emailCount == 1 ? '1 e-mail was sent during this test.' : '@count e-mails were sent during this test.';

d'oh!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Would you consider making interdiffs please?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new interdiff.txt750 bytes

Sorry, but in general, I'm not doing interdiffs for trivial patches like this here, because an interdiff requires me to execute a complex shell command on the command line, whereas I do not touch the command line at all for simple patches like this. You're asking for too much, and it's not me who's guilty, our d.o/git infrastructure is.

As pointed out in #12, I overlooked a stale "@count" replacement token of the previous format_plural().

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This may be a trivial patch for you, but anyone who has already reviewed one version and then sees a whole new patch and maybe doesn't want to read through the whole thing again appreciates an interdiff. Thanks!

Anyway... Obviously I missed that problem on my earlier review. I looked through the code changes here again carefully, and I think they are all good this time.

olli’s picture

olli commented

#2 did WebTestBase too. Is there another issue for that one?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

There's no issue for WebTestBase yet. I'm actually not sure whether it needs to be converted (aside from consistency reasons perhaps), because WebTestBase operates with a fully available service container.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

It would be good to convert if for no other reason than to remove strings from localize.d.o that don't really need to be translated.

But actually this issue, as filed, was supposed to address that. The title was changed in #9 just a few days ago... Can we just change it back and add the fixes for the other test base classes as was the original intention?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Can we move that into a separate issue? Baby steps... The changes of the existing patch here should be sufficient to unblock #1808220: Remove run-tests.sh dependency on existing/installed parent site

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

OK feel free, but please update the issue summary and file the other issue, since the current patch doesn't address the current issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
Parent issue: » #500866: [META] remove t() from assert message
Related issues: +#1808220: Remove run-tests.sh dependency on existing/installed parent site

Created #2195623: Remove t() from default test assertions in WebTestBase class and updated issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Excellent. In which case this patch is back to RTBC. Thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Posted #2196241: Remove string translation services from TestBase container as a follow-up.

Status: Fixed » Closed (fixed)

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