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

Update contents of https://www.drupal.org/docs/develop/automated-testing/phpunit-in-drupal/... with the following

Assertion messages

The default assertion message provided by PHPUnit is sufficient for most situations. The page explains when a custom assertion message can be added and the format of the message.

When to add a custom assertion message

  1. When the assertion in inside a loop.
  2. In a custom assertion method in a trait or base test class. A custom assertion method begins with public function assert*();.
  3. In a helper method.

Format of a custom assertion message

%subject% should %verb% %payload%

Definitions

%subject%

The additional context information

%verb%

The action.

%payload%

The value the%subject% should be.

Example

foreach ($expected_steps as $element_id => $step_value) {
  foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) {
    $name = $element_id . '[' . $sub_field . ']';
    $expected_step = $step_value[$sub_field] ?? 1;
    $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.");
  }
}

  where 

{subject} == "Duration input $name"
should == 'should' ;)
{verb} == 'have'
{payload} == 'the correct step value'

--- original proposal ---

When to use a custom assertion message

  1. 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.
  2. 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.)
  3. 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...

Positive messages (what we're expecting if it passes)
  1. Most assertion messages in core are already written this way (as a hold-over from the SimpleTest days).
  2. Symfony does it this way. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Con... for example.
  3. 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.");
  4. ...
Negative messages (what went wrong if it failed)
  1. 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.
  2. 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."
  3. ...
Be Explicit: Declare the meaning as part of the message

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."

Pros
  1. Even more closely matches PHPUnit's default messages.
  2. No chance of ambiguity or misunderstanding.
  3. Self-documenting.
  4. ...
Cons
  1. More work: every custom assertion message will have to be changed in all of core - nothing does it this way (yet).
  2. More repetition: every custom message will start with the same 23 chars.
  3. 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".
  4. ...
Say what's intended, but phrase it conditionally (so it makes sense when it fails)

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.");
      }
    }
Pros
  1. No repetition.
  2. The failure messages all make sense.
  3. The test code is self-documenting and makes sense.

Remaining tasks

  1. Continue to flesh out reasons for/against each approach
  2. Decide which approach we're going to standardize on
  3. Figure out where to document it
  4. Decide proper scope for child issues
  5. Open all the child issues
  6. Fix all the child issues to make core consistent about this
  7. 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.

Comments

dww created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

@dww, thank you for filing this!

@mondrake:

1) In Simpletest, the message was a description of what the assertion was checking, so that in the Simpletest UI this could be visually presented as a result of the test run. In PHPUnit, on the contrary, the message is a description of the failure occurred while testing, so that it can be presented in the log and then test execution stops.
In the conversion, the messages should be 'negated', for example:

$this->assertInstanceOf(ViewExecutable::class, $factory->get($view), 'A ViewExecutable instance was returned from the factory.');

For Simpletest the message was OK, but in PHPUnit, if the assertion fails, it would be because the $factory->get($view) is NOT an instance of ViewExecutable::class, so the message should be, really, 'The object returned from the factory was not an instance of ViewExecutable.'

So to say that in HEAD currently the messages were not adjusted as part of conversions and are in many cases misleading in case of test failures.

2) The standard message generated by PHPUnit in case of failure of an assert(Not)InstanceOf is probably more than enough in 99% of the cases. GIven the case above, failure of

$this->assertInstanceOf(ViewExecutable::class, $factory->get($view));

will return something like

$ phpunit InstanceOfTest
PHPUnit 9.0.0 by Sebastian Bergmann and contributors.

F

Time: 0 seconds, Memory: 5.00Mb

There was 1 failure:

1) InstanceOfTest::testFailure
Failed asserting that 'WhateverClass" Object (...) is an instance of class "ViewExecutable".

/home/sb/InstanceOfTest.php:6

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

-- @see the issue summary of #3130811: Remove redundant $message from assert(Not)InstanceOf calls

@jungle:

+1 to do this,

  1. The assertion, assertInstanceOf / assertNotInstanceOf, is self explained. To me, the optional message is not necessary, rarely read it.
  2. And even if the message is important, as a fallback an inline comment is expected and better for eyes -- read from up to down, not left to right, to avoid horizontal scrolling.

-- @see issue #3130811: Remove redundant $message from assert(Not)InstanceOf calls, comment #6

@catch:

fwiw I'm +1 to removing custom assertion messages from the majority of our assertions - a lot of the assertion messages were added when we originally added Simpletest to core, and have just been ported through to phpunit.

-- @see issue #3130396: Replace assertions involving calls to is_file with assertFileExists()/assertFileNotExists(), comment #13

Quoting opinions form @mondrake and @catch, FYI. And reposting my comment here, to sum up my own opinions/suggestions:

  1. Most of the assertions are self-explained, the extra message is unnecessary usually. Especially with the improvement from ongoing #3128573: [meta] Replace assertions with more appropriate ones
  2. Aggressively, to adopt the no message way, using inline comment when an explanation is really needed.
  3. About the scope, I would suggest:
    a) Per pair of assertions (For example, assertIsArray()/assertIsNotArray()).
    b) Or per group of assertions (For example, assertIs*(), including assertIsString(),assertIsNotString(), assertIsArray(), assertIsNotArray() etc.)

    And I think it's acceptable having larger patches if we go for a larger scope eventually, even though it's not encouraged, As all are tests related, nothing won't break.

  4. Worthy thinking to add phpcs sniffers to generate patches automatically, but it relays on the new release(s) of coder, if so, I am afraid it will take longer to finish unless creating one sniffer to get them all
dww’s picture

dww
we/he/they
commented

@jungle: Indeed, as I wrote in the 2nd paragraph of the summary:

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, and independent of this plan. 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.

Deciding if we need custom messages doesn't really matter what kind of assertion it is, it matters where the assertion lives. If there's no loop, and the line number is enough to know what failed, remove the message and rely on the default. But in any case where there's a loop, or something else that makes the line number insufficient to know exactly what assertion failed, we need a custom message (not an inline comment). In those (and only those) cases, we need to have a clear policy for how the messages are written.

Thanks,
-Derek

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented 
  public function testLoopDemo() {
    $data = [0, 'b', 'c'];
    foreach ($data as $index => $item) {
      $this->assertEquals($index, $item);
    }
  }

Added the demo purpose code which contains a loop, to core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php. the following is the error message.

Testing Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest
F                                                                   1 / 1 (100%)

Time: 944 ms, Memory: 4.00MB

There was 1 failure:

1) Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest::testLoopDemo
Failed asserting that 'b' matches expected 1.

/var/www/html/core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php:88

FAILURES!
Tests: 1, Assertions: 2, Failures: 1.

I think combining the error message with the line number, that's enough/sufficient to debug.

I do not understand why the loop matters and needs to be discussed specially. Maybe if there is a loop, the test needs to be refactored to use data provider.

Thanks!

dww’s picture

dww
we/he/they
commented

For a concrete example where #4 is definitely not sufficient, check out the Kernel test here: #3020676-11: Support the #date_increment property in the 'duration' form element.

The test is a form, and the form contains a ton of 'duration' elements, each of which contains 6 actual form inputs. The test does this:

    $form = \Drupal::formBuilder()->getForm($this);
    $this->render($form);

    $expected_steps = [
      'duration_date_increment_5sec' => ['s' => 5],
      'duration_date_increment_1min' => ['i' => 1],
      'duration_date_increment_5min' => ['i' => 5],
      'duration_date_increment_15min' => ['i' => 15],
      'duration_date_increment_1hr' => ['h' => 1],
      'duration_date_increment_2hr' => ['h' => 2],
      'duration_date_increment_7hr' => ['h' => 7],
      'duration_date_increment_1day' => ['d' => 1],
      'duration_date_increment_3day' => ['d' => 3],
    ];
    foreach ($expected_steps as $element_id => $step_value) {
      foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) {
        $name = $element_id . '[' . $sub_field . ']';
        $expected_step = $step_value[$sub_field] ?? 1;
        $input = $this->xpath("//form//input[@name='$name']");
        $this->assertCount(1, $input, "Duration input $name not found.");
        $actual_step = (integer) $input[0]->attributes()->{'step'};
        $this->assertEquals($actual_step, $expected_step, "Input $name has the wrong step value.");
      }
    }

Without those custom messages, if something fails, you'd have no way to know what element or step was broken relying only on the default message from assertCount() or assertEquals().

So, then we're back to the question at hand... should the message say:

A) "Duration input $name exists."

B) "Duration input $name not found."

C) "Failed asserting that duration input $name exists."

?

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Make sense, @dww, thanks!

$this->assertCount(1, $input, "Duration input $name not found.");

How about defining the variable in the message inside a loop as a context, if there is at least one context in the message inside a loop, the message stays. Otherwise, it seems ok to remove.

catch’s picture

catch
he/him
Primary language English
commented

Standardising, as (or in the process of) removing as many custom assertion messages as possible makes sense.

If we switch to negative, then I think we should adopt the 'Failed asserting that...' syntax from phpunit. phpunit is really doing Failed to assert that [positive statement] rather than [negative statement inverting the meaning of the assertion]. Just the negative statement might sometimes work when you're reading test results, but when reading the test code it's very confusing.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#7 please note though that on assertion failure, PHPUnit will print out BOTH the custom message AND the standard message, for example

PHPUnit 8.5.3 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\big_pipe\Functional\BigPipeTest
.F..                                                                4 / 4 (100%)

Time: 22.8 seconds, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\big_pipe\Functional\BigPipeTest::testBigPipe
BigPipe start signal does not appear before stop signal.
Failed asserting that 13239 is less than 11896.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:116
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:54
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:2887
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:969
/var/www/html/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php:402
/var/www/html/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php:184
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

FAILURES!
Tests: 4, Assertions: 161, Failures: 1.

So a custom message starting with 'Failed asserting...' will be a bit of a repetition.

In web assertions, currently we have a message that represents what was found and what was expected, for example

PHPUnit 8.5.3 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest
.EEEEEEE.                                                           9 / 9 (100%)

Time: 1.01 minutes, Memory: 4.00 MB

There were 7 errors:

1) Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPublic
Behat\Mink\Exception\ExpectationException: Current response header "Content-Length" is "193", but "193" expected.

/var/www/html/vendor/behat/mink/src/WebAssert.php:768
/var/www/html/vendor/behat/mink/src/WebAssert.php:161
/var/www/html/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:211
/var/www/html/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:77
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

and such message cannot be overridden, since Mink does not manage custom messages.

But I think that could be a good way forward - fact-based - to be adopted directionally: Found {this} in the context of {whatever}, but {that} was expected.

My 2c.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Elaborating a bit on #8, also to reflect a practical attempt we launched in #3131816: Replace assertions involving calls to is_array() with assertIsArray()/assertIsNotArray(), a suggestion could be:

{something} is expected to {verb} {subject} [, but {actual}].

Examples:

      $this->assertIsArray($this->definitions[$type], "Plugin type '$type' is expected to have an array of plugins.");

      $this->assertNotEmpty($this->definitions[$type], "Plugin type '$type' is expected to have plugins."); 

      $this->assertContains($expected_type_plugins[$a], $this->definitions[$type], "Plugin type '$type' is expected to contain the '$a' list of plugins, but that is missing."); 

      $this->assertEquals($expected_type_definitions[$a], $this->definitions[$type], "Plugin type '$type' definition is expected to be equal to the list in $a.");

      $this->assertEquals($aaa[$a], $bbb[$a], "Current response header '$a' is is expected to be $bbb[$a], but $aaa[$a] was found.");
dww’s picture

dww
we/he/they
commented

Thanks for all the input and feedback, everyone!

@jungle Re: #6: I don't quite follow what you mean with this:

How about defining the variable in the message inside a loop as a context, if there is at least one context in the message inside a loop, the message stays.

Isn't that what the example I posted in #5 is doing? Not sure your point.

@catch Re: #7: 💯agree. That's exactly what the Be Explicit option in proposed resolution is about. That's my favorite, too.

@mondrake Re: #8: I think a little repetition ("Failed asserting that") is fine. It's way preferable to raw negation messages (which make the test code itself extremely confusing), IMHO.

Re: #9: Yeah, I could possibly get behind {something} is expected to {verb} {subject} [, but {actual}].. But maybe "Failed asserting that [positive]" is sufficient (and easier to port core to be using), especially since the default message (that's going to include actual vs. expected in 99% of the cases) will handle those details for us.

Again, I think the primary use-case for custom messages is to handle assertions inside loops where a @dataProvider doesn't make sense. So the main point of any remaining custom messages would be to identify which loop iteration actually failed. If we're going to document the resulting policy, I think we should be explicit about this point.

Thanks again,
-Derek

dww’s picture

dww
we/he/they
commented
Title: [META] [policy] Standardize PHPUnit assertion messages to be either 'positive' or 'negative' (not the mix of both we have now) » [META] [policy] Remove PHPUnit assertion messages when possible, but standardize remaining messages to be either 'positive' or 'negative' (not the mix of both we have now)

Based on Slack flack from @moshe, giving this a more accurate title... ;)

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
moshe weitzman’s picture

moshe weitzman commented
Title: [META] [policy] Remove PHPUnit assertion messages when possible, but standardize remaining messages to be either 'positive' or 'negative' (not the mix of both we have now) » [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages to be either 'positive' or 'negative' (not the mix of both we have now)
moshe weitzman’s picture

moshe weitzman commented
Title: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages to be either 'positive' or 'negative' (not the mix of both we have now) » [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Does the message even have to be positive or negative? Can it just state the useful facts and nothing else?

$this->assertFieldByXPath("//form//input[@name='$name' and @step='$step']", NULL, "Form input $name with step $step");

$input = $this->xpath("//form//input[@name='$name']");
$this->assertCount(1, $input, "Form input $name");

$this->assertIsArray($this->definitions[$type], "Plugin type '$type'");
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Re #10, in #6 i was saying a pattern to identify assertion messages in a loop which should stay. the pattern is if there is a variable in assertion message, the assertion should stay. Because i found the difference between #4 and #5 is one has $variable in assertion message, the other does not. To me, such variables are contexts.

Don't be surprised if what I said in English is weird, :p.

Thanks!

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Failed to assert that [positive statement]

Has a big +1 from me.

chi’s picture

chi commented

One more reason to remove assertion messages.
https://github.com/sebastianbergmann/phpunit/issues/4198

neclimdul’s picture

neclimdul
He/Him
commented

Oh this is a nice issue! Some opinions.

I'm a pretty big -1 on adding "Failed to assert that..." boiler plate. I get that is sets the tone for the message but encourages repeating the assertion phpunit is going to provide for coding standard completeness and provides little meaning leaving us back where we are now.

100% on board that we over use messages. Phpunit's assertion messages are _generally_ very good too and I've been seeing recent commits converting bad "equals/same" and "true/false" assertions to more appropriate assertions so kudos to those developers for laying some ground work on this because that's kind of key :)

I think longwave is right, "positive" isn't a great descriptor but the intent is accurate. Providing context is the key, it needs to be affirmative contextual information not a negative statement for all the reasons pointed out.

Example time. The count example is kinda the ideal case for messages improving the context. Especially if you modify it slightly. Changing

$this->assertCount(1, $input, "Form input $name");

to

$this->assertCount(1, $input, "Instances of form input $name");

You then know both what and why we're counting. For example you immediately know 0 means "oh the the field didn't show up?" or 2 means "oh 2 instances? I duplicated the field?" The existing message would have you digging in the test to see why the test cares about "Form input X"

Back to the boiling plate, phpunit will give us an error like:

Instances of form input 'title'
Failed asserting that actual size 0 matches expected size 1.

While the suggested format would be something like:

Failed to assert instances of form input 'title'
Failed asserting that actual size 0 matches expected size 1.

I guess its open to opinion which is better, the second does provide the extra context but I think it more reads repetitive with the redundant messaging not providing any additional meaning.

dww’s picture

dww
we/he/they
commented

@neclimdul re: #19: Thanks for your thoughts. For the record, in the summary I'm proposing:

E.g. instead of: "Field X exists" vs. "Field X not found", the message should say "Failed asserting that field X exists."

I agree "Failed to assert instances of form input 'title'." is weird.

That's why I like "Failed asserting that field X exists.". Although as you point out, the assertion could also fail if the xpath found 3 instances. ;) /shrug

Regardless, I'm glad we seem to have universal agreement that we should remove the custom message most of the time, and that work is already underway, so yay. Once that's complete, we'll have a much smaller # of custom messages to still deal with, so whatever we decide will be easier to implement.

Thanks everyone. Please keep the suggestions coming. :)

Cheers,
-Derek

dww’s picture

dww
we/he/they
commented

More examples from my Kernel test at #3020676: Support the #date_increment property in the 'duration' form element. that's partly what prompted me to open this issue. ;)

Here's another proposal:

    foreach ($expected_steps as $element_id => $step_value) {
      foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) {
        $name = $element_id . '[' . $sub_field . ']';
        $expected_step = $step_value[$sub_field] ?? 1;
        $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.");
      }
    }

If you break the code for the first case, you see this:

There was 1 failure:

1) Drupal\Tests\duration_field\Kernel\DurationFormElementTest::testDurationElement
Duration input duration_date_increment_2day[s] should appear exactly once.
Failed asserting that actual size 0 matches expected size 1.

If you break the code for the 2nd case, you see:

There was 1 failure:

1) Drupal\Tests\duration_field\Kernel\DurationFormElementTest::testDurationElement
Duration input duration_date_increment_2day[d] should have the correct step value.
Failed asserting that 3 matches expected 2.

A) No repetition.
B) The failure messages all make sense.
C) The test code is self-documenting and makes sense.

I think this is a pretty good compromise on all the various concerns. To generalize the recommended form:

{context} should {verb} {subject}

{context} == "Duration input $name"
should == 'should' ;)
{verb} == 'have'
{subject} == 'the correct step value'

Thoughts? Should we call this a 4th option in the summary? It's sort of a cross between the 'positive' and 'explicit' cases.

Thanks,
-Derek

neclimdul’s picture

neclimdul
He/Him
commented

I'm leery of strict rules in documentation but maybe this isn't that. Either way I think you perfectly translated my thoughts into a clear format. Thanks and one vote for that! :-D

mondrake’s picture

mondrake
Location 🇮🇹
commented

Just c/p here a post from #3131348-34: Replace assertions involving calls to empty() with assertEmpty()/assertNotEmpty()/assertArrayNotHasKey(), purely to foster discussion. Thanks.

for me it's completely unclear when a message should stay or not at the moment.

I am not in a better place :)

I can only say my rule of thumb at the moment - just based on few threads here and there. That's the one I followed for #3131348-30: Replace assertions involving calls to empty() with assertEmpty()/assertNotEmpty()/assertArrayNotHasKey().

  1. 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.
  2. In custom assertion methods (i.e. those starting with public function assert*();) contained in traits or base test classes, I try to 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.)
  3. 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 could be used in messages to provide context.
  4. There is not a final word on what a message should contain. @dww started a standardization discussion in #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages , that the place to discuss. Personally, I am sticking to the proposal in #3131946-21: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages , and using a pattern {context} should {verb} {subject} to craft the messages.
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Re #23.2 and maybe #23.3 this was my intent behind #3133288: [meta] Add custom PHPUnit assertions for common tests - for the example given we could convert assertCacheKey() to a real PHPUnit assertion and then the default message would be useful ("Failed to assert that the cache key...") and the $message argument only needed to provide extra context.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#24 yes, +1. I actually tried a first stab at that in #3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative.

There are hundreds of custom assertions in test code, try git grep 'function assert'. Trying and 'PHPUnitizing' them will be a major effort...

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Re: #23 That list for when to have a custom message looks great to me! Copying points 1-3 into the proposed resolution in a new When to use a custom assertion message subsection.

Also, added what seems to be our current favorite solution as a new Say what's intended, but phrase it conditionally option under If needed, what kind of custom message should we use?.

Thanks,
-Derek

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented

Correct me if I am wrong, but shouldn't this be moved to the Coding Standards project?

neclimdul’s picture

neclimdul
He/Him
commented

Maybe. I think the current running consensus on this also doesn't match the title or issue summary.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

xjm’s picture

xjm
she/her
Primary language English
commented

Re: #29, core can adopt a best practice without it being the standard for all contrib as well, which is what the coding standards project is for.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Project: Drupal core » Coding Standards
Version: 11.x-dev »
Component: phpunit » Coding Standards

Moving to Coding Standards project for discussion.

quietone’s picture

quietone commented
Project: Coding Standards » Drupal core
Version: » 11.x-dev
Component: Coding Standards » other

On second thought, this is best in core. Sorry for the noise.

neclimdul’s picture

neclimdul
He/Him
commented
Title: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages » [META] [policy] Improve assertion messages when possible, and standardize remaining messages

I think this is stalled pretty hard since the discussion is getting close to 5 years. However, I feel like there was pretty strong consensus around settings some guidance at least that messages should be positive/"confirmative"/affirmative additional messages, letting the assertion show the negative/failure information and the message provide the context for what the failure means.

In the interest of closing issues, could we more forward with just that and if there's apatite for removing or suggesting the removal that could be handled separately? I'm biased because I had strong feelings but I feel like that was always the part we where aggressively agreeing over.

neclimdul’s picture

neclimdul
He/Him
commented
Title: [META] [policy] Improve assertion messages when possible, and standardize remaining messages » [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages

Didn't mean to change the title. I started to take a stab and didn't have enough caffeine in my system yet to have a good suggestion.

quietone’s picture

quietone commented

Where do we want to document this?

quietone’s picture

quietone commented

It is still not clear where this should go. It could be added as a new page in the guide. PHPUnit in Drupal. That would encourage the practice now and, if desired, an issue can be opened in Coding Standards to formally accept the practice.

I read through the issue to create a a bare minimum proposed text for the changes, which follows. What do you think?

Assertion messages

The default assertion message provided by PHPUnit is sufficient for most situations. The page explains when a custom assertion message can be added and the format of the message.

When to add a custom assertion message

  1. When the assertion in inside a loop.
  2. In a custom assertion method in a trait or base test class. A custom assertion method begins with public function assert*();.
  3. In a helper method.

Format of a custom assertion message

%subject% should %verb% %payload%

Definitions

%subject%

The additional context information

%verb%

The action.

%payload%

The value the%subject% should be.

Example

foreach ($expected_steps as $element_id => $step_value) {
  foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) {
    $name = $element_id . '[' . $sub_field . ']';
    $expected_step = $step_value[$sub_field] ?? 1;
    $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.");
  }
}

  where 

{subject} == "Duration input $name"
should == 'should' ;)
{verb} == 'have'
{payload} == 'the correct step value'
quietone’s picture

quietone commented
Issue summary: View changes

Move the proposal from previous comment to the issue summary

quietone’s picture

quietone commented
Status: Active » Needs review
quietone’s picture

quietone commented
Issue summary: View changes

I put the content in https://www.drupal.org/docs/develop/coding-standards/drupal-simpletest-c...

neclimdul’s picture

neclimdul
He/Him
commented

"The default assertion message provided by PHPUnit is sufficient for most situations."
Still strongly disagree with this.

Something like "Failed asserting that false is not equal to false." is entirely insufficient and such assertion failures are most situations.

xjm’s picture

xjm
she/her
Primary language English
commented

This issue has come a long way since the last time I looked at it -- nice work everyone!

I think the one thing that I might change is to start off with a stronger statement about not using assertion messages. Possibly framing it as a coding standard, something like "Custom test assertion messages MUST NOT be used, unless one of the following is true": or something along those lines. Along with the why: Using custom assertion messages actually hides valuable debugging information and misleads the developer. We want them to read the test.

Thoughts?

xjm’s picture

xjm
she/her
Primary language English
commented

Oh, I see it's already in the handbook. I'll make an edit there directly and post back here for review.

xjm’s picture

xjm
she/her
Primary language English
commented

Added some revisions.

That should make it clearer why it is preferred not to use assertion messages unless they are absolutely necessary. I also made it clearer what kind of "assertion message" we are talking about, since the word "assertion" is very overloaded in our ecosystem and might be interpreted to mean other things.

Edit: Although it looks like I'm going in the opposite direction from @neclimdul. @neclimdul, it's our (core committers') experience that folks' custom assertion messages more often hide more than they reveal, and it actually makes debugging harder. The preferred workflow is to open the test and look at the line where the assertion failed to understand what it means when FALSE is TRUE.

We definitely often mark core issues NW asking for the assertion message to be removed, probably for 8 or more years now.

xjm’s picture

xjm
she/her
Primary language English
commented

For reference, this issue came up in the context of work done for #3128573: [meta] Replace assertions with more appropriate ones, which actually required removing many many assertion messages throughout the core codebase in huge cleanup efforts in order to unblock those conversions. Broadening the scope from the removals required by those conversions to a general policy was sort of several different streams of practice converging, since we already had a long history of removing redundant assertion messages as a reason to NW core issues, and frustrations from core devs in debugging when bad assertion messages misled them.

xjm’s picture

xjm
she/her
Primary language English
commented

@cmlara raised a concern that, technically, the handbook page is in the coding standards section as if this is already an approved coding standard that applies to all of contrib as well as core, whereas it's only the draft of a policy formalizing core practice for an NR core issue as per #41. Not sure what to do about that.

quietone’s picture

quietone commented

The page was originally in the PHPUnit in Drupal guide titled, "Drupal SimpleTest coding standards". At the time, and with no other contributor feedback, it made sense to move it to Coding Standards.

I will move it back in a bit.

quietone’s picture

quietone commented
Issue summary: View changes

I fixed my error by moving the page back, it is at https://www.drupal.org/docs/develop/automated-testing/phpunit-in-drupal/....

alexpott’s picture

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

This issue concerns me. For me, making this a coding standard would be a mistake. There's far too much nuance at play. For example - look at the assertions being added by #3567544: Alias lookup by path is not case sensitive but AliasPrefixList and AliasRepository::preloadPathAlias's return value is leading to inconsistencies

    $this->assertEquals($path_alias->getAlias(), $aliasManager->getAliasByPath($path_alias->getPath()), 'Basic alias lookup works.');
+    $this->assertEquals($path_alias->getAlias(), $aliasManager->getAliasByPath(strtoupper($path_alias->getPath())), 'Basic alias lookup is case-insensitive.');

The first assertion above is in HEAD and the second is being added. I agree that the first message is completely pointless. But the second message is important. The default assertEquals message is not going to clue you in that the purpose of this assertion is about case-insensitivity.

Secondly, assertFalse, assertTrue, assertNull messages are useless and it could be argued that the opposite of this issue should be enforced - adding context for why you are making that assertion is useful.

I think this issue had more merit back in the simpletest days when using a custom message meant that you often lost the default message from the assertion that was often more useful especially when testing equivalence. But with PHPunit custom messages are appended to the default message so that is no longer an issue.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Title: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages » [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages
Status: Needs review » Reviewed & tested by the community

Since about 2012 the document included 'coding standard' in the title. But this is not a coding standard, it is a best practice or guidelines about the assertion messages. Today, I changed the title to be descriptive that it only about assertion messages and when to use them.

The document is in the Develop section so can be improved by the community on an ongoing basis. If anyone wants to have a standard for the assertion message that can be enforced, open an issue in the Coding Standard project. Thanks.

I have updated credit and setting to RTBC.

catch’s picture

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

Yes I think this is fine, we've moved away from custom assertion messages in a lot of places but equally we can't enforce not having them because sometimes it makes things easier if the default one would be too cryptic.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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