Document that t() should not be used for assertion message texts and groups

.

Until core conforms to this standard, I don't see a reason why we should document this, nor be knocking back patches to "needs work" that don't conform. Also, if we do this, we need crystal clear instructions for how to do things like t('Created the @foo type', array('@foo' => $foo)) because right now, people can use the same thing they know how to do everywhere, and this proposal means they'd need to learn something special that's specific to tests.

Postponing on #500866: [META] remove t() from assert message.

This issue applies only to the assertion message texts, not to matched strings.

The alternative to t('Created the @foo type', array('@foo' => $foo)) is format_string() and is documented here:
http://drupal.org/simpletest-tutorial-drupal7#t
Edit: Fixed. Edit: It's also not unique to tests; it's an API function.

Reactivating this now that 8.x is most of the way to compliant.

OK... Some thoughts:

a) We have a standards page on tests:
http://drupal.org/node/325974
I don't see it mentioning this idea of not using t() in assertion messages, so we need to write that up and add it to that page.

b) Someone should go through the entire section of http://drupal.org/simpletest and make sure that
- All of the examples are correct [no t() in assertion messages]
- In any documentation about using the assert methods, we mention this.

c) We should make sure the Examples for Developers project complies with this standard. (should file an issue there)

d) I'm not sure that I agree that we need to say "Don't translate $message with t()" in every assertion method documentation... but I'm not totally against the idea either. What's the proposed wording?

So, other places in core where a passed-in string should not be passed through t():

• watchdog() -- unfortunately, I don't think that's actually a good example...
• hook_menu() -- Simply says "The untranslated X" to imply that t() should not be added.

The current text in assert():

   * @param $message                                                            
   *   The message string.

In assertTrue() and other TestBase assertions:

  * @param $message                                                            
   *   The message to display along with the assertion.                         

In various WebTestBase assertions:

   * @param $message                                                            
   *   Message to display. 

So, how about something like:

   * @param $message                                                            
   *   (optional) The untranslated message to display with the assertion. Use
   *   format_string() to embed variables in the message text. If left blank, a
   *   default message will be displayed.

We could additionally simply add the word "untranslated" to the $group parameter description, but maybe that's a followup issue.

Two thoughts:
- Let's take care of both $message and $group here (that is not much scope creep).
- When we say "untranslated" in other places in core docs, we're usually implying that it will be picked up by the PO generator and that it will be translated before being displayed. So how about wording like this:

$message

(optional) A message to display with the assertion. Do not translate messages: use format_string() to embed variables in the message text, not t(). If left blank, a default message will be displayed.

$group [currently says, at least on some methods: The type of assertion - examples are "Browser", "PHP".]

(optional) The group this message is in, which is displayed in a column in test output. Use 'Debug' to indicate this is debugging output. Do not translate this string. Defaults to 'Other'; most tests do not override this default.

updated summary

I just updated the issue summary's proposed resolution section with a list of tasks that need to be completed for this issue (and I filed a separate issue for the Examples for Developers project).
#1803844: Remove t() from test assertion messages

That parameter documentation looks good to me as well.

I agree that the proposed @param documentation looks great. Thanks @jhodgdon!!

OK, then we need a patch -- three agreements on proposed documentation wording is probably enough, since the accuracy of the statements is not really in question. :)

I've filed this as an office hours task.

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

Uses messages from #9 and replaces them in TestBase.php and WebTestBase.php

Had leftover pieces from another patch in my diff. Here is a new version...

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -163,9 +163,14 @@ protected function checkRequirements() {
+   *   (optional) The group this message is in, which is displayed in a column
+   *   in test output. Use 'Debug' to indicate this is debugging output. Do not
+   *   translate this string. Defaults to 'Other'; more tests do not override
+   *   this default.

I think this should be "most tests" rather than "more tests"?

Related: #1601146: Allow custom assertion messages using predefined placeholders

Good call. Changed xjm.

Thanks @kbasarab!

This has been committed to 8.x. We should backport to 7.x...

But first I went back to look at these files. This patch needs a follow-up to fix:

a) WebTestBase::assertTextHelper -- check $group - it doesn't appear to have a default in this function, so that needs to be taken back out of the docs.

b) Same for WebTestBase::assertUniqueTextHelper

c) How come WebTestBase::assertThemeOutput() has no $group parameter? It should. Also the docs should note that $message is altered in the function (has stuff added to the end, check the code for details).

d) WebTestBase::assertFieldByName() also has no $group parameter (has one in the docs though). It should. Also assertNoFieldByName() and assertFieldById() and assertNoFieldById() and assertFieldChecked() and assertNoFieldChecked() and assertOption, assertNoOption, assertOptionSelected, assertNoOptionSelected, assertResponse, assertNoResponse(), assertMail

e) assertMailString doesn't even let you pass in your own message, much less group.

f) assertMailPattern has $message but it is ignored.

So... Some parts of this are probably separate issues. But the parts where the docs that were added don't agree with the functions should be fixed here. That would be (a), (b), fixing $message in (c), removing $group docs for functions in (d) that have no $group parameters. The rest is a follow-up issue, which I've just filed:
#1817144: Some test assertions are inconsistent with the others

c) I'm not sure I'm seeing how message is altered. Code looks to provide its default message with a format_string argument for the callback function. I don't see anything being appended though if you do specify a $message:

     if (!$message) {                                             
       $message = '%callback rendered correctly.';
     }                                                            
     $message = format_string($message, array('%callback' => 'theme_' . $callback . '()'));

e) assertMailString doesn't document $group or $message currently.

Changes are attached. I was a bit confused as to which ones should be in that separate issue as some of those you mentioned needing fixes here are listed in separate issue. Take a look at this patch and I'll adjust from there.

I may jump on the new one as well if I get some time this weekend or tomorrow so we can get these changes in.

Oh, you're right on (c). Maybe we should document that if you put %callback into your message, something will be substituted?

On (a), the text needs to be re-wrapped in your patch, but otherwise looks good.

On the other stuff... sorry for putting all of that there -- it was confusing and I should not have put all those notes there (I was just collecting them as I went)...

As far as the functions that don't have $group arguments, I think we should leave adding the arguments to the other issue, since this one is only supposed to affect documentation and not code. So this issue, at least for the moment, ought to remove $group documentation from functions that don't currently have $group arguments, so that the documentation matches the code. I'm not sure what action, if any, will be taken on that other issue (unless you'd like to go over there and do a patch! :) ).

Also, this patch shouldn't be adding a default of '' to $message for functions that didn't have it.

So basically: the objective here is to make the documentation match the current code, and as this is an issue in the "documentation" component, we should not be changing any code lines, only documentation. Thanks!

Alrighty. Cleaned up the code additions and will tackle those in #1817144: Some test assertions are inconsistent with the others. Removed $group docs from the ones that don't have $group specified yet and rewrapped the assertNoText and assertNoUniqueText group block.

Forgot to change the $message for the %callback.

Fixes tests.

Thanks!

That last patch has two problems that I noticed on a quick read:

-   *   messages: use format_string() to embed variables in the message text, not
-   *   t(). If left blank, a default message will be displayed.
+   *   messages: Using "%callback" will be replaced by "theme_$callback()" in
+   *   output. If left blank, a default message will be displayed.

I think we still need to mention format_string(). I would put the following at the end of the original text:
If you use %callback as part of your message, it will be replaced by the theme call that was used in the output.

+   * @param $message
+   *   A message to display with the assertion. Do not translate
+   *   messages: use format_string() to embed variables in the message text, not
+   *   t().
+   *   *

Stray * (this is the last block in the patch).

I removed the format_string because in the code we have:

2402     if (!$message) {
2403       $message = '%callback rendered correctly.';
2404     }
2405     $message = format_string($message, array('%callback' => 'theme_' . $callback . '()'));
2406     return $this->assertIdentical($output, $expected, $message);

format_string is being called by default so you would be formatting string twice if the user used it. I filed a case related to this that addresses that and fixes that to be like other assertions: #1818124: assertThemeOutput $messages not same format as other assertions

Second part I will get fixed. Not sure why the test failed on that. I'll look into that further when I get a chance.

#27: document_t_assertion-1457320-27.patch queued for re-testing.

I am think the test failure might be a random one. I saw a similar error with the same test class earlier today. Hence, submitted #27 for re-testing.

Removes the stray * from this one.

One little thing to fix: The new docs for $group are missing the (optional) that was there before. Can that go back in?

Also, I don't think the new wording for assertThemeOutput reads quite right:

* @param $message
    *   (optional) A message to display with the assertion. Do not translate
-   *   messages: use format_string() to embed variables in the message text, not
-   *   t(). If left blank, a default message will be displayed.
+   *   messages: Using "%callback" will be replaced by "theme_$callback()" in
+   *   output. If left blank, a default message will be displayed.
    *

How about making it say:

(optional) A message to display with the assertion. Do not translate messages: use format_string() to embed variables in the message text, not t(). "%callback" in $message will be replaced by "theme_$callback" in the output. If left blank, a default message will be displayed.

The thing is, yes, as you noted above, format_string() is called within the method. But still if someone wants to put *other* stuff into the message, they should use format_string too. Calling it twice will not cause any problems.

And finally, I realized when reading this patch that the last bit that adds documentation for $message to assertMailPattern... actually looking at the code, $message is totally ignored. So we should probably leave the @param but make the documentation say "Ignored".

Thanks jhodgdon.

Made adjustments you recommended. Also added the (ignored) in for $message in assertMailPattern. Spawned a new issue in #1832618: assertMailPattern ignores $message parameter for allowing $message to be customized.

#34: document_t_assertion-1457320-34.patch queued for re-testing.

Thanks! This looks right, but I did notice one misspelling of "column" that is missing the n at the end...

And I think this will conflict with #1817144: Some test assertions are inconsistent with the others (which I think might make this follow-up patch obsolete, yes?), so maybe we should postpone doing this follow-up patch at all?

status forgot.

add more to dos to proposed resolution

#34: document_t_assertion-1457320-34.patch queued for re-testing.

assertNoFieldByXPath still has the $group parameter, but in patch 34, it had that param section being removed. I changed it to trim it, like was done for assertNoText and assertNoUniqueText.
Same thing with assertFieldByName. In patch 34, the group param was removed in the function definition, but in current release, it is back in. I did the same trim as above.
Same with assertNoFieldByName.
And the assertMailString doc was already changed.
And for assertNoUniqueText, the $be_unique param is not in the function definition, so I ommitted that changed line.

Returning to this old issue...

Looking at the issue summary here, it seems like the scope of this issue, for Core patch, would be to document that message and group parameters to simpletest assertions should not be translated.

This patch isn't doing that. It looks like it was fixed elsewhere actually. So I think we don't need a patch at all.

What still needs to be done here:

A lot of the sub-pages of https://www.drupal.org/simpletest have t() on messages. These pages need editing. Badly!

Actually, on closer look, this has already been fixed on the docs pages.