[META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages

Moving to Coding Standards project for discussion.

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

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.

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.

Where do we want to document this?

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'

Move the proposal from previous comment to the issue summary

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

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

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.

Thoughts?

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

Added this revision:
https://www.drupal.org/node/325974/revisions/view/13972755/14026145 →

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.

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

For reference, this issue came up in the context of work done for 🌱 [meta] Replace assertions with more appropriate ones Fixed , which actually required removing many many assertion messages throughout the core codebase in huge cleanup efforts in order to do 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.

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

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.

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