Allow custom assertion messages using predefined placeholders

See also #1601202: Make simpletest assertions output both the default and custom message texts.

PHPUnit has the same assertSame() and assertNotSame(), so this test makes complete sense. I also like that we're already making use of it in some cases.

Looking at PHPUnit's assertNotSame(), I see why it's not "!==". RTBC.

If we switch to PHPUnit at some point, what will happen to these?

I just marked #1803674: Remove t() from default test assertions in TestBase class as a duplicate of this issue. It was taking a simpler approach...

+      if (strpos($message, $token) === FALSE) {
+        unset($args[$token]);
+        continue;
+      }

Why is this required?

I looked at the code here... a couple of comments:

a) Why is this @todo here?

+      // @todo Remove all manual var_export() calls from tests and remove this.
+      if (!is_string($value)) {
+        $args[$token] = var_export($value, TRUE);
+      }

It seems like it would actually be fairly common for one of the tokens to be an ordinary string variable value, so I think you would always need this? Also, some of the previously existing code used var_export on string data, such as this bit from assertNoFieldByXPath:

-        $message = t('Found field with name @name', array(
-          '@name' => var_export($name, TRUE),

b) I think it would be useful to document for each $message what placeholders someone can use if they override the default message. For instance on assertTrue/False, document that @value will be replaced by the value in any message, and on assertEqual, @first and @second will be replaced... but maybe that's a separate issue since many of these placeholders already existed.

c) in tearDown()

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

This message is still being translated. format_plural() does translation.

d) in drupalCreateRole()

-          $this->pass(t('Created permissions: @perms', array('@perms' => implode(', ', $permissions))), t('Role'));
+          $this->pass($this->formatString('Created permissions: @perms', array('@perms' => implode(', ', $permissions))), 'Role');

Does it still make sense to implode, since var_export() is being used in formatString()? [this applies to other parts of the patch too]

re @20, I might argue that if either the actual OR the expected had a newline, then *both* should be wrapped in PRE, just for consistency of the visual comparison, no?

I am not certain that most/all users of assertEqual put the "actual" value first and the "expected" value second, either? It doesn't seem right to me. assertEqual previously had the semantics of "see if these two values are equal", not "see if this value is what you expect it to be". I don't know that it's always used in the "actual vs. expected" manner. I think it would be better to leave the message as less specific as it used to be.

Re #22: Can you explain in what use-case you would use that function that isn't the actual/expected use-case? Both in my own usage and also when reading other people's code that has been the exclusive one I've seen.

Well, at a minimum I am not sure that "actual" and "expected" are always in the order that is suggested by this modification.

Ahh, okay, I can see that. But don't you think that could be a matter of getting used to the new function? IDE people will even get the autocomplete to tell them what to put where. And to me it feels natural to do $actual first and then $expected, because that is the way we do if{}s in core. I.e. we avoid yoda conditions such as if ('Adam' == $user->name()) but instead we do if ($user->name() == 'Adam').

Sorry for being a little bit persistent, but I see great value in a function as depicted in #20. I wrote a rather comprehensive suite of tests for Libraries API 2.x which consisted of comparing very long arrays, where the $expected value was (minimally) computed, and even after manually inserting the < pre > tags, as that function also does, it was very hard to remember which browser tab was which, and, by extension whether my test was wrong or whether it was actually showing a legit fail. Another example is you see a failed assertion on "NULL is equal to FALSE." which tells absolutely nothing about what is going on.

How many core and contrib tests are doing it the other way and would therefore need to be changed?

Note that PHPUnit signatures have $expected as the first argument.

http://www.phpunit.de/manual/3.8/en/writing-tests-for-phpunit.html#writi...

Huge swaths of the previous patch do not apply any more. And I still think it needs work due to the comments above.

Updated issue summary.

I've been writing unit tests for Drupal\simpletest\TestBase. You know, like you do. And t() and other globals make it a pain to test.

I might be hijacking this issue with something a bit out of scope, but regardless of whether its t() or format_string(), this class needs to be unit tested completely, and therefore it can't be a global.

I suggest following the lead of this: https://drupal.org/node/2079611

Basically have a TranslationInterface object as a property, allowing TestBase to have its own t() method. Then replace t() with $this->t().

Doing it correctly allows for dependency injection in unit tests:

  protected function t($string, array $args = array(), array $options = array()) {
    if (empty($this->stringTranslation)) {
      $this->stringTranslation = \Drupal::translation();
    }
    return $this->stringTranslation->translate($string, $args, $options);
  }

The main problem with this approach is that some methods in TestBase are inexplicably static (insertAssert() mainly), and don't know what $this means so t() remains global within them.

Static baaaad. :-)

#30 is a completely separate issue. This one is really only about test assertion messages, not about other translations. Please open a new issue. Thanks!

Right. Test assertion messages are currently run through t(). The issue is using str_replace() and format_string() instead, in order to allow predefined placeholders.

I point out that t() and format_string() make it difficult to write unit tests, so please do dependency injection the right way.

Which is not off-topic.

The title of this issue was misleading, so I have updated it to reflect the current issue summary. I think we have fixed all the test assertions' use of t(), on a separate meta-issue. If there are still t() calls that shouldn't be there, that is a separate issue and should be referencing the other meta-issue.

This issue is specifically about making the base tests assertions have a "base string" argument that would be passed through format_string() so that standard substitutions could be made.

OK. :-)

And here's your other issue.. :-)

#2144669: Improve/Refactor TestBase Through Expanded Unit Testing

Re #33: This issue is referenced by #500866: [META] remove t() from assert message.

EDIT: reopened #1803674: Remove t() from default test assertions in TestBase class

I would like to reroll the patch #30, but not exactly clear to me what is the expected result. (Assertion message).
Do we need replace the String::format() with $this->t() ?
And what about the format_plural()?

RE #36, the patch in #30 is out of scope, see #31 ... I really don't know what it was trying to do, but we absolutely do **not** want to be translating test assert messages. We spent a huge amount of effort on several other issues to make sure we were not translating them, and as far as I can tell, the patch in #30 is trying to translate them again. I don't know why.

We probably need to go back to the patch in #17, get it rerolled, and address the comments in #19-#25, rather than starting with the patch in #30.

I think the isset() returns TRUE for empty strings.

$message = '';
if (!isset($message)) {
  // Never step into here.
}

The default value of the $message argument in most of the assert*() method is an empty string, which is checked with if (!isset($message)) {, therefore I changed the check to if (!$message) {.

I think it would be better to change the default value of the $message argument to NULL from empty string.

Yeah, a patch that passes the tests! Going forward, please include an interdiff with each new patch, so we can see what changed. For now, we can use the patch in #41 as a baseline.

So, I think this looks pretty good. I have a few suggestions for the documentation:

a) In the formatString() method docs, it refers to format_string() but it actually passes the formatting to String::format().

b) assertTrue() docs:

+  /**
+   * Check to see if a value is not false (not an empty string, 0, NULL, or FALSE).

- Check ==> Checks

c) All assert methods using the new formatString() calls: We need to document what the default message and placeholders are, in the @param $message for each method, so that users of that assertion get this information. And we can probably take out the part that is in all of them about using String::format().

As an example, I would suggest this for assertTrue:

@param $message

(optional) A message to display with the assertion, which defaults to 'Value @value is TRUE.' You can use @value as a placeholder in your message for the $value parameter (it will be passed through var_export() and appropriately formatted). Do not translate messages.

d) I am not sure why some of the messages are using ! and some are using @ for formatting. I think we should pick one and use it consistently.

Hmm... Given the code in the TestBase::formatString() method, which sometimes inserts PRE tags, I think @ is not a good idea. Wouldn't @-formatting take those PRE tags and do check_plain() on them, which would make them not be tags any more? So probably we should be using ! instead of @ in the default messages that are inserting the input variables? Maybe we should test this and see what happens if you do:
assertTrue(array('foo' => 721));
or something like that.

e) Some of the changes in this patch aren't all that great:

-      $message = String::format('Current URL is @url.', array(
-        '@url' => var_export($this->container->get('url_generator')->generateFromPath($path, $options), TRUE),
+      $message = $this->formatString('Current URL is @url.', array(
+        '@url' => url($path, $options),

We still need to do $this->container->get('url_generator')->generateFromPath() not url() here.

.

We should still consider doing this, but it would be 8.1 now.

This would be so valuable, let's do this!

I believe this is no longer relevant, phpunit assertions displays both the message and the values, maybe some of our legacy/BC assertions could be improved but should mostly be fine.