use the message argument of assertX functions

[SimonHeimberg]

Assert functions from PHPUnit do have an optional message argument. This is especially usefull for assertTrue because the message printed on failure does not (and can not) contain any details. Demonstrate this possibility

[OskarStark]

indention looks weird

[SimonHeimberg]

agree, ) should be at the same level as $this->...

[wouterj]

it should be either:

$this->assertTrue(
    $client->getResponse()->isSuccessful(),
    'Response status code is 2xx'
);

Or just everything on one line.

[OskarStark]

I like this PR 👍

Status: Reviewed

[javiereguiluz]

My comments about this:

• I agree that adding the message to assertXXX() is a good practice...
• ...except in cases like this $this->assertTrue($client->getResponse()->isSuccessful()); I find this code very readable and the explanation looks a bit redundant.
• Besides, I always write assert messages in a positive way (Status code is 2xx) so I find it a bit confusing to read negative messages (Status code is NOT 2xx). I don't know which is the most common practice here.

[lyrixx]

My two cents: I never use $this->assertTrue($client->getResponse()->isSuccessful());. I really prefer $this->assertSame(200, $client->getResponse()->getStatusCode()); ; Like that it's easier to spot the kind of error (404 vs 403 vs 500). It's a small tips, but it's very convenient (and shorter btw)

[SimonHeimberg]

My motivation to write assert messages is not readable code. It is the failure message.

Example failure without assert message:

1) PostControllerTest::testSomeFunctionality
Failed asserting that false is true.

/home/aUser/PostControllerTest.php:19

Without checking the source code at the mentioned line I do not know what went wrong. (Did isRedirect() fail or is the content type wrong?) By using assertSame(200, ...) or assertEquals I can guess that Failed asserting that 200 is 404 reports about http status (but no idea about the url).
The problem gets bigger on long functional tests.

I prefer a failure message like this

1) PostControllerTest::testSomeFunctionality
"Content-Type" header is NOT "application/json"
Failed asserting that false is true.

/home/aUser/PostControllerTest.php:19

Write the assert message positive or negative or ...? I do not know which of this messages is clearer. Some suggestions:

1. "Content-Type" header is NOT "application/json"
2. "Content-Type" header is "application/json"
3. expect "Content-Type" header "application/json"
4. ...

[wouterj]

I agree with @javiereguiluz that I prefer to use positive messages. It seems to be the common practice in Symfony core as well.

👍 for adding this messages btw, it makes things more clear in the PHPunit test report.

[wouterj]

Oh, can you please update your PR? (I know it's from a long time ago, if you don't want to do it anymore/have no time now feel free to say so and we'll take over).

Status: needs work

[wouterj]

Hi @SimonHeimberg! I took your commit and added a new one to make the messages positive. See #6736

Thank you for making these changes! I'm going to close this one in favor of the new PR.