Problem/Motivation
Core uses PHPUnit for our automated tests. All PHPUnit assertion methods allow for an optional custom message. In PHPUnit, these messages are only displayed when a given assertion fails. If no message is defined, the assertions provide a default message themselves.
In many cases, the best solution is to remove the custom message and let the default message from PHPUnit appear when the assertion fails. Removing duplicate or extraneous messages is worthwhile whenever possible. However, there are definitely still cases where a custom message is helpful or even necessary. For example, an assertion inside a loop needs a custom message so you know which loop iteration you were on when the assertion failed.
PHPUnit itself is un-opinionated on whether these messages should be written in a "positive" (aka "confirmative") way (what we're expecting to be true, which was violated if the assertion fails) or a "negative" one (what went wrong that caused the assertion to fail). All PHPUnit itself gives us is this:
https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/C...
* @param string $description Additional information about the test
Back when we used SimpleTest, all the assertions came with a manually crafted assertion message. These messages were displayed in the UI to show what passed or failed. The message was green if the assertion passed, and red if it failed. So it made sense for us to always write the messages in the "positive" form. Most test assertions in core, when ported to PHPUnit, kept the messages phrased for the positive case.
It doesn't really matter if we interpret the additional information about the assertion failure as negative or positive. But what we have now is a really confusing mix of both. Sometimes, the message explains what failed (negative). Most of the time, it explains what we were expecting (positive).
Proposed resolution
When to use a custom assertion message
- In a normal test method (i.e. those starting with
public function test*();), do not add messages UNLESS you are in a loop AND you want a failure to report context information that is not present in the assert arguments.
- In custom assertion methods (i.e. those starting with
public function assert*();) contained in traits or base test classes, have a message saying what's expected. This is because that test code is executing there as a result of an assert* call from a normal test method, and it's useful to report 'where' you are when the test is failing. (purely for example - it's more useful to report that you are missing a cache key if you have called an assertion like assertCacheKey(), since a message 'Failed to assert that an array has a key' would be too vague.)
- Similarly if you are asserting within a helper method. The helper method would be called from within test methods with some arguments, and the content of those arguments should be used in messages to provide context.
If needed, what kind of custom message should we use?
We need to pick one of these, document it, and then open child issues to make sure all of core does it consistently. Assuming that the pro for one approach is a con for the other, I'm just collecting reasons to pick a choice in the list under each sub heading...
- Most assertion messages in core are already written this way (as a hold-over from the SimpleTest days).
- Symfony does it this way. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Con... for example.
- It arguably makes the test code easier to read/understand, since the message helps document what the assertion is testing for. It's generally easier for our minds to make sense of things phrased positively, instead of having to invert meanings and understand things as negations. For example:
$this->assertFieldByXPath("//form//input[@name='$name' and @step='$step']", NULL, "Found form input $name with step $step.");
or
$input = $this->xpath("//form//input[@name='$name']");
$this->assertCount(1, $input, "Found form input $name.");
- ...
- The message is only printed if something failed, so it makes sense that the message would be saying what went wrong, not what we were hoping was true.
- Default PHPUnit assertion messages (when we don't define our own) are generally phrased this way. For example:
"Failed asserting that 0 matches expected 1."
- ...
Instead of picking positive or negative and relying on convention or documentation for it to make sense, we could adopt PHPUnit's basic approach, and start all assertion messages with "Failed asserting that..." so readers immediately understand the message, instead of having to know how to interpret it. E.g. instead of: "Field X exists" vs. "Field X not found", the message should say "Failed asserting that field X exists."
- Even more closely matches PHPUnit's default messages.
- No chance of ambiguity or misunderstanding.
- Self-documenting.
- ...
- More work: every custom assertion message will have to be changed in all of core - nothing does it this way (yet).
- More repetition: every custom message will start with the same 23 chars.
- The default assertion messages are always displayed, even if a custom message is defined. So there's also repetition for each failed assertion where we'd have 2 errors that both start with "Failed asserting that".
- ...
See #21:
Custom assertion messages will have the form:
{subject} should {verb} {payload}
For example:
foreach ($expected_steps as $element_id => $step_value) {
foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) {
$input = $this->xpath("//form//input[@name='$name']");
$this->assertCount(1, $input, "Duration input $name should appear exactly once.");
$actual_step = (integer) $input[0]->attributes()->{'step'};
$this->assertEquals($expected_step, $actual_step, "Duration input $name should have the correct step value.");
}
}
- No repetition.
- The failure messages all make sense.
- The test code is self-documenting and makes sense.
Remaining tasks
- Continue to flesh out reasons for/against each approach
- Decide which approach we're going to standardize on
- Figure out where to document it
- Decide proper scope for child issues
- Open all the child issues
- Fix all the child issues to make core consistent about this
- Rejoice
User interface changes
N/A -- only changes to the output when test assertions fail.
API changes
N/A
Data model changes
N/A
Release notes snippet
TBD. Probably not. Only impacts developers.