Optional custom fail messages in asserts

[MartinMystikJonas]

Sometimes test results can be hard to understand without detail examination of test code.

For example:

Assert::true($obj->isA());
Assert::true($obj->isB());

You just get something like:

   Failed: FALSE should be TRUE

   in Tester/Framework/Assert.php(370) 
   in Tester/Framework/Assert.php(141) Tester\Assert::fail()
   in tests/common/SomeTest.phpt(18) Tester\Assert::true()

Only information about what caused tha failure is line number on last line.

Also there are some cases when assertion is executed multiple times (for example asserting something for every item in collection) where we need to know for which item test failed.

Therefore it is usefull to have option to add custom fail messafe to asserts.

Assert::true($obj->isA(), "Object is A");
Assert::true($obj->isB(), "Object is B");

And then you get:
You jsut get something like:

   Failed: Object is B: FALSE should be TRUE

   in Tester/Framework/Assert.php(370) 
   in Tester/Framework/Assert.php(141) Tester\Assert::fail()
   in tests/common/SomeTest.phpt(18) Tester\Assert::true()

It could save you lot of time in examining test results.

[MartinMystikJonas]

There is failure in Travis build:

-- FAILED: tester/tests/Dumper.dumpException.phpt
   Exited with error code 255 (expected 0)
   E_RECOVERABLE_ERROR: Object of class stdClass could not be converted to string

   in Tester/Framework/Assert.php(442) 
   in Tester/Framework/Assert.php(56) Tester\Assert::failMsg()
   in tester/tests/Dumper.dumpException.phpt(23) Tester\Assert::same()
   in [internal function]{closure}()
   in Tester/Framework/Assert.php(285) call_user_func()
   in tester/tests/Dumper.dumpException.phpt(27) Tester\Assert::exception()

Reason is that in test Dumper.dumpException is used call to Assert:same with three parameters instead of two.

line 23 | ... Assert::same(str_repeat('x', 100), str_repeat('x', 120), new stdClass); },

I'm not completely sure about purpose of this test so I didn't fixed it yet. Why is there call to same with three parameters? It there third parameter just by mistake or is there any reason for that?

[dg]

It is typo, fixed 4d68141, you can rebase it

[dg]

Related #48

[fprochazka]

there should be a space after if, and if the != is intentionally not !== you should write it in a comment

[MartinMystikJonas]

Rebased and conding standard fixed

[hrach]

We do not use abbreviations, so failMsg shoudl be failMessage.

[MartinMystikJonas]

Renamed failMsg to failMessage

[hrach]

why space?

[MartinMystikJonas]

typo I will fix it

[hrach]

Few notes: missing phpdoc param - only where there are some defined :) and also we use ', if there is no need for " :) Butw that are just nits.

[MartinMystikJonas]

@hrach Fixed

[milo]

@MartinMystikJonas My opinion is negative on this (#48) :) Imho it just blocks position for another more useful parameters.

I'm solving it in my tests as asserting arrays like:

$actual = [];
foreach (...) {
    $actual[] = ....;
}
Assert::same([TRUE, TRUE, TRUE], $actual);

[MartinMystikJonas]

@milo In my opinion basic assert method doesn't need any more parameters. And if need for such parameter arises then we can just create mew method.

Assert::same($expected, $actual, "My message")
Assert::someWithParam($expected, $actual, $param, "My message")

Have custom message as optional parameter is common convetion in other testing frameworks. For example:
http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.assertions.assertEquals
[http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String, long, long)](http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals%28java.lang.String, long, long%29) -there is this optional parameter on first place because we can detect that in Java
http://www.ruby-doc.org/stdlib-2.1.1/libdoc/test/unit/rdoc/Test/Unit/Assertions.html#assert_equal-method

[hrach]

@MartinMystikJonas 💯

[milo]

@MartinMystikJonas I know this convention but still I don't like it. Few more reasons:

• it is not related to assertion arguments, it is user notice/helper,
• you almost don't use it,
• not so hard to emulate it by own Assert class.

If so, I would rather to see something crazy like:

Assert::section('Testing object A')->true(....);

I'll accept it if @dg merge this, but I'll not vote fot it.

[hrach]

@milo this is not about clean code, etc, this is about readability and not inventing the wheel. Proposed attitude is well known. Again, please, do not invent the wheel.

[MartinMystikJonas]

@milo

• This message describes assertion intention. I would say it is related to it's arguments. It's the same reason why we have exceptions with messages and not only identified by their type.
• I use it quite frequently (currently in by own Assert class 😄 )
• I was quite surprised it is not supported when I first use nette/tester because it is really common
• Acording to some sources good fail message is most important part of the test if you practice TDD

[milo]

This message describes assertion intention. I would say it is related to it's arguments. It's the same reason why we have exceptions with messages and not only identified by their type.

Exception message is compared with expected message. It is an alone assertion. Fail message is only the message.

@MartinMystikJonas Do you use own fail message for all assertion types or only for some e.g. true() false() null()?

[MartinMystikJonas]

@milo It was meant bit different with exception message. I meant usage in application code. For program purposes only exception type is relevant. But we still use descriptive exception messages as help for developers examinig these exceptions. The same reasoning should apply to Assertion failures in tests.

I use own fail messages if there is more than one assertion in test. If there is only one assertion the test name should be enough description of problem. Also I do not use own fail messages when default fail message is good enough to identify the problem. For example assertion of thrown exception is usually descriptive enough - there is not much more information you can provide other than different than expected exception was thrown. But for most basic assertions you need aditional information to know what exactly failed.

In my opinion test output should be descriptive enough so you doesn't need to check test code to know what failed.

[milo]

@MartinMystikJonas Oh, I understand now.

At all, I guess I'm using different techniques than you but I understand to your needs.

[MartinMystikJonas]

Yes, you probably use tests only for regression testing. Am I right? In this use case you are basically only interested if test fails or not and test fails only exceptionally.

But if you use TDD techniques (or something similar like me) you start by failing tests which run many times during development and you need quick feedback what changes in code changed in tests results.

[hrach]

I heard TDD is dead

[MartinMystikJonas]

@hrach Heard that too 😄 And with most points of DHH I agree. Luckily I'm using only something inspired by TDD not it's strict fundamentalist version.

[milo]

@MartinMystikJonas Time to time I use kind of TDD but usually with one or two tests at the moment and running them by browser with Tracy.

[MartinMystikJonas]

@milo I usually run them directly from console in IDE.

[dg]

Current solution when you see 123 should be TRUE and line number is really not sufficient.

In my opinion is ideal when test Assert::true($obj->isA()); will fail with message like $obj->isA() is 123 but should be TRUE. And I believe it can be done, in most cases, withnout user message.

[MartinMystikJonas]

@dg Seeing which assertion caused the failure is good enough for many cases. Solution in #134 is perfect for these cases.

But there are still cases when you need include additional information about failure context. For example Assertions executed inside cycle or recursive function. Or simply when you need to know value of some other variables not only the one you checked in assertion.

[MartinMystikJonas]

It this PR still considered as merge candidate?

[fprochazka]

This feature is very usefull, I would like to see this merged.
@MartinMystikJonas can you please rebase your pullrq? Do you mind writing few tests?

[MartinMystikJonas]

@fprochazka If there is real chance for this to be merged I will surely rebase it and write tests.

[dg]

I think it is useful.

[dg]

I know that message is commonly used, but would not it be more accurate to use description?

Btw, use please single quotes where possible. For optional arguments is the best to use NULL.

[MartinMystikJonas]

I can rename it but message is common name in this context.

I will change it to single quotes

[MartinMystikJonas]

Rebased, fixed and tests added.

[milo]

👍 One more thing, update commit message please to be consistent.

I'm thinking, if a longer backtrace has some influence for somebody, but probably not.

[dg]

failMessage is imho bad name. Is new method really needed?

[milo]

Idea: what about passing array($description, '....') to fail and concat here?

[MartinMystikJonas]

From my POW separate this to method is better. It is only private so it will not change signature of class and I think it improves readability. Maybe rename it to failWithDescription?

@milo: This solution expects all parts of message to be equivalent. But I think they are not and in future there could be different processing for mail reason and fail description (f.e. different colors in output).

[vojtech-dobes]

Maybe stupid idea, but if some method is necessary, what about failBecause?

[MartinMystikJonas]

@milo: What change in commit message do you mean?

[dg]

Conversation about failMessage is here #133 (comment) but is hidden in outdated diff.

[milo]

@dg I see!

@MartinMystikJonas
Commit message like Assert: added custom fail description
And as you wrote, private self::message($reson, $description) seems OK to me.

[dg]

Verb is better. What about describe() ?

[MartinMystikJonas]

@dg: changed to descibe()

@milo: Commit message updated

[dg]

👍

[milo]

@MartinMystikJonas please, add tests for notSame, notEqual, contains, falsey and 💯

[MartinMystikJonas]

done

[dg]

Thanks

[milo]

@MartinMystikJonas Thank you!