You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by Vladimir Sitnikov <si...@gmail.com> on 2019/02/28 13:44:04 UTC
Code review vs unit tests vs assert usage
Hi,
I happen to maintain software, and I see a common pitfall when it comes to
unit tests.
Please don't take it personally, however please consider it when creating
new tests and/or when reviewing patches.
I'm afraid the below might be too long, however below applies to JMeter
committers, to JMeter contributors, and it applies to any developers in
general.
TL;DR:
General rule: failing test should look like a good bug report. Thus
Assert.fail() is bad.
Consider using "single assertion" per test method. Having separate test
methods helps manual execution of the tests, and it makes test report
cleaner
Consider using assertEquals(String message, expected, actual) instead of
assertTrue(expected == actual). The former allows you to provide human
readable message and it integrates well with IDEs (i.e. it allows to open
diff of expected and actual).
If using just assertTrue(expected == actual) all you get is a stacktrace
and if such a test fails a developer has to reverse engineer the intention
behind that code.
== Problem statement ==
Sample test code (it was added as a part of Bug 62785):
SampleResult sample = sampler.sample();
assertFalse(sample.getResponseDataAsString().contains("java.io.FileNotFoundException:"));
Even though the test makes sure the code works, there's a maintenance
problem with this test.
When the test fails it is very cryptic.
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertFalse(Assert.java:64)
at org.junit.Assert.assertFalse(Assert.java:74)
at
org.apache.jmeter.protocol.http.sampler.TestHTTPSamplers.testRawBodyFromFile(TestHTTPSamplers.java:385)
Of course we see that assertFalse was violated, however it is absolutely
obscure why the test failed and it is obscure what it was trying to do.
== Desired behaviour ==
Test failure should look like a good bug report.
For instance, if one can take a decent bug report by a simple copy&paste
"AssertionError" plus the stacktrace, then the assert is good.
== Possible approaches ==
Note: this all depends on a case-by-case basis.
A) Use message parameter of assert* calls.
For instance: assertEquals("Response message should not contain
FileNotFoundException", false,
sample.getResponseDataAsString().contains("FileNotFoundException:"));
Note it would still produce quite weird output like "expected false got
true".
In general, assertEquals for comparing booleans is almost always wrong.
B) use if + assert.fail
if (sample.getResponseDataAsString().contains("FileNotFoundException:")) {
Assert.fail("Response must not contain FNFE");
}
C) Use Hamcrest matches. For instance:
assertThat(sample.getResponseDataAsString(),
not(containsString("java.io.FileNotFoundException:")));
Of course "C" might be less verbose in this case, however there's
**extremely** important improvement which "C" brings over A and B (it can
be fixed though, but I made a deliberate mistake there).
...
...
Ok, you are great if you discovered that: assertThat print actual response
data in case the assert fails. In other words, we would know what was the
response.
D) Gradle + Spock might improve readability as well (see
https://twitter.com/VladimirSitnikv/status/1100666414548045825 )
== Ok, I use message/assertThat, is it enough? ==
No. assertThat does not always help.
For instance, take a look into TimerServiceTest
https://github.com/apache/jmeter/blob/3d03af3d78746d67e7a81d2481e5e741b3907664/test/src/org/apache/jmeter/timers/TimerServiceTest.java#L34
public void testBigInitialDelay() {
long now = System.currentTimeMillis();
long adjustedDelay = sut.adjustDelay(Long.MAX_VALUE, now + 1000L);
// As #adjustDelay uses System#currentTimeMillis we can't be sure, that
the value is exact 1000L
Assert.assertThat(Math.abs(adjustedDelay - 1000L) < 150L,
CoreMatchers.is(true));
}
Does the test look good to you?
...
...
Felix?
...
Ok, if you can spot the error, you are great.
The problem here is even though assertThat is used, test failure won't
explain the nature of the test. It won't explain WHY "true" was expected
and so on.
Proper assert should be like the following:
if (...) { // Or Spock or whatever
Assert.fail("TimerService.sut.adjustDelay(Long.MAX_VALUE, now + 1000L)
should return something close to 1000 since now+1000 would anyway be less
than Long.MAX_VALUE. Actual result is " + adjustedDelay);
}
== Next steps ==
0) The most common code smell is "computations in assert* messages".
1) "Constant" messages do not really help.
For instance, assertEquals("Incorrect entry returned",expected[i], out[i]);
is a bad message.
It does not explain "why" something was expected. What was the input? What
was the action? Nothing of that is present in "incorrect entry returned"
message.
2) If you create a test assertion or an assert in the production code,
think how the failure look like.
You might even want to temporary put wrong values and see if the message is
understandable and actionable.
3) If you review the patch, please pay attention on the way asserts are
written.
Note: reviewer often sees only positive side of the test. In other words,
CI says that tests pass. However reviewer does NOT see how the failure
would look like in CI.
Great applause to everybody who reach this line. Comments welcome.
Vladimir
Re: Code review vs unit tests vs assert usage
Posted by Vladimir Sitnikov <si...@gmail.com>.
sebb> This could be turned into a Wiki or other web-page so it can be
sebb>readily referenced and enhanced.
It could, however I have a secret desire to create a talk on that
topic, so I don't want to spoil it by publishing an article before the
talk :)
sebb> In this specific case, adding the value of 'i' to the message would help.
No, adding "i" won't help because it still won't explain WHY the
expected value is as provided.
In other words, "Incorrect entry at index 5. expected: d, actual: z"
is still a very bad assertion message.
Imagine a Bugzilla report: Bug 374562: Incorrect entry at index 5.
expected: d, actual: z
Longer version is below.
Here's the full code (see TestCSVSaveService):
private void checkStrings(String[] expected, String[] out) {
assertEquals("Incorrect number of strings
returned",expected.length, out.length);
for(int i = 0; i < out.length; i++){
assertEquals("Incorrect entry returned",expected[i], out[i]);
}
}
private void checkSplitString(String input, char delim, String[]
expected) throws Exception {
String[] out = CSVSaveService.csvSplitString(input, delim);.....
checkStrings(expected, out);
}
checkSplitString("a,bc,d,e", ',', new String[]{"a","bc","d","e"});
In fact, the assertions are "completely broken" (tm)
Let's assume we have a bug so the actual array is {"a", "bc,d", "e"}
checkStrings above would fail with "incorrect number of strings
returned expected: 4, got: 3"
Does it help? Not really.
Let's assume we have a bug so the actual array is {"a", "bc", "d", "z"}
checkStrings above would fail with "Incorrect entry returned expected: e got: z"
Does it help? Not really.
Does adding index to the message help? Not really.
The proper code would be to remove checkStrings completely, and update
checkSplitString as follows
private void checkSplitString(String input, char delim, String[]
expected) throws Exception {
String[] out = CSVSaveService.csvSplitString(input, delim);.....
assertEquals("csvSplitString(\""+input+"\", delim="+delim+")",
Arrays.toString(expected)
+ ", length=" + expected.length, Arrays.toString(out) +
", length=" + out.length);
}
In case of a failure it would print full expected output, full actual
value, and (it is very important!) it will print the input arguments
for csvSplitString.
So the failure would look like: csvSplitString("a,bc,d,e", delim=,)
expected: {a, bc, d, e}, length=4, actual: {a, bc, d, z}, length=4
Vladimir
Re: Code review vs unit tests vs assert usage
Posted by sebb <se...@gmail.com>.
On Thu, 28 Feb 2019 at 13:44, Vladimir Sitnikov
<si...@gmail.com> wrote:
>
> Hi,
>
> I happen to maintain software, and I see a common pitfall when it comes to
> unit tests.
> Please don't take it personally, however please consider it when creating
> new tests and/or when reviewing patches.
>
> I'm afraid the below might be too long, however below applies to JMeter
> committers, to JMeter contributors, and it applies to any developers in
> general.
>
> TL;DR:
>
> General rule: failing test should look like a good bug report. Thus
> Assert.fail() is bad.
>
> Consider using "single assertion" per test method. Having separate test
> methods helps manual execution of the tests, and it makes test report
> cleaner
>
> Consider using assertEquals(String message, expected, actual) instead of
> assertTrue(expected == actual). The former allows you to provide human
> readable message and it integrates well with IDEs (i.e. it allows to open
> diff of expected and actual).
>
> If using just assertTrue(expected == actual) all you get is a stacktrace
> and if such a test fails a developer has to reverse engineer the intention
> behind that code.
Agreed.
Tests should not use non-specific fail messages or messages which
don't show the expected and actual values.
Ideally add the test before fixing the bug, and check that the failure
message is clear about the bug.
Then fix the bug so the test passes.
> == Problem statement ==
>
> Sample test code (it was added as a part of Bug 62785):
>
> SampleResult sample = sampler.sample();
> assertFalse(sample.getResponseDataAsString().contains("java.io.FileNotFoundException:"));
>
> Even though the test makes sure the code works, there's a maintenance
> problem with this test.
> When the test fails it is very cryptic.
>
> java.lang.AssertionError
> at org.junit.Assert.fail(Assert.java:86)
> at org.junit.Assert.assertTrue(Assert.java:41)
> at org.junit.Assert.assertFalse(Assert.java:64)
> at org.junit.Assert.assertFalse(Assert.java:74)
> at
> org.apache.jmeter.protocol.http.sampler.TestHTTPSamplers.testRawBodyFromFile(TestHTTPSamplers.java:385)
>
> Of course we see that assertFalse was violated, however it is absolutely
> obscure why the test failed and it is obscure what it was trying to do.
>
> == Desired behaviour ==
>
> Test failure should look like a good bug report.
> For instance, if one can take a decent bug report by a simple copy&paste
> "AssertionError" plus the stacktrace, then the assert is good.
>
> == Possible approaches ==
>
> Note: this all depends on a case-by-case basis.
>
> A) Use message parameter of assert* calls.
> For instance: assertEquals("Response message should not contain
> FileNotFoundException", false,
> sample.getResponseDataAsString().contains("FileNotFoundException:"));
>
> Note it would still produce quite weird output like "expected false got
> true".
> In general, assertEquals for comparing booleans is almost always wrong.
>
> B) use if + assert.fail
> if (sample.getResponseDataAsString().contains("FileNotFoundException:")) {
> Assert.fail("Response must not contain FNFE");
> }
>
> C) Use Hamcrest matches. For instance:
> assertThat(sample.getResponseDataAsString(),
> not(containsString("java.io.FileNotFoundException:")));
>
> Of course "C" might be less verbose in this case, however there's
> **extremely** important improvement which "C" brings over A and B (it can
> be fixed though, but I made a deliberate mistake there).
> ...
> ...
> Ok, you are great if you discovered that: assertThat print actual response
> data in case the assert fails. In other words, we would know what was the
> response.
>
> D) Gradle + Spock might improve readability as well (see
> https://twitter.com/VladimirSitnikv/status/1100666414548045825 )
>
> == Ok, I use message/assertThat, is it enough? ==
> No. assertThat does not always help.
> For instance, take a look into TimerServiceTest
> https://github.com/apache/jmeter/blob/3d03af3d78746d67e7a81d2481e5e741b3907664/test/src/org/apache/jmeter/timers/TimerServiceTest.java#L34
>
> public void testBigInitialDelay() {
> long now = System.currentTimeMillis();
> long adjustedDelay = sut.adjustDelay(Long.MAX_VALUE, now + 1000L);
> // As #adjustDelay uses System#currentTimeMillis we can't be sure, that
> the value is exact 1000L
> Assert.assertThat(Math.abs(adjustedDelay - 1000L) < 150L,
> CoreMatchers.is(true));
> }
>
> Does the test look good to you?
> ...
> ...
> Felix?
> ...
> Ok, if you can spot the error, you are great.
>
> The problem here is even though assertThat is used, test failure won't
> explain the nature of the test. It won't explain WHY "true" was expected
> and so on.
>
> Proper assert should be like the following:
> if (...) { // Or Spock or whatever
> Assert.fail("TimerService.sut.adjustDelay(Long.MAX_VALUE, now + 1000L)
> should return something close to 1000 since now+1000 would anyway be less
> than Long.MAX_VALUE. Actual result is " + adjustedDelay);
> }
>
> == Next steps ==
>
> 0) The most common code smell is "computations in assert* messages".
>
> 1) "Constant" messages do not really help.
> For instance, assertEquals("Incorrect entry returned",expected[i], out[i]);
> is a bad message.
> It does not explain "why" something was expected. What was the input? What
> was the action? Nothing of that is present in "incorrect entry returned"
> message.
In this specific case, adding the value of 'i' to the message would help.
If the test case name is properly chosen, that can also help (e.g. if
it refers to the bug report).
> 2) If you create a test assertion or an assert in the production code,
> think how the failure look like.
> You might even want to temporary put wrong values and see if the message is
> understandable and actionable.
See my comment above about adding the test before fixing the bug.
Start with the test, see what the output looks like, and then fix the
code so the test works.
If you design the test first, you are less likely to be influenced by
the code, and so more likely to properly test the desired
functionality.
Functional tests should be written from the specification.
[Additional tests will be needed for full code coverage, and these
obviously depend on the implementation.]
> 3) If you review the patch, please pay attention on the way asserts are
> written.
> Note: reviewer often sees only positive side of the test. In other words,
> CI says that tests pass. However reviewer does NOT see how the failure
> would look like in CI.
If possible, apply the tests in the patch first.
Run the tests and check the messages make sense.
Also check the expected tests fail - i.e. they detect the failure.
Then add the main code and check the tests pass.
> Great applause to everybody who reach this line. Comments welcome.
This could be turned into a Wiki or other web-page so it can be
readily referenced and enhanced.
> Vladimir
Re: Code review vs unit tests vs assert usage
Posted by Vladimir Sitnikov <si...@gmail.com>.
Does it include output? Yes
Felix> Your original version of "B" included the response string to make it
Felix> more verbose and more tester friendly, so they are -- in my eyes --
Felix> rather equivalent in output.
A will print:
AssertionError: Response message should not contain FileNotFoundException
at ...
at ...
B will print
AssertionError: Response must not contain FNFE
at ...
at ...
C will print something like
AssertionError: the string shot not contain "FileNotFoundException",
but the actual value was "..>><<<!!!!.. "
In this regard, C is way better since it enables to cross-check the
output even without looking into the sources.
For instance, the actual FileNotFoundException might include some
weird file name or path or whatever.
In that regard options A&B just print "there was some error"
Note, one can use A* which would be "as good as C":
A*) Assert.fail("Response must not contain FNFE, actual response was:
" + sample.getResponseDataAsString());
Felix>which I expect a tester to be familiar with. I find myself often
Felix> struggling to find the right words to state the expectation of the
Felix> assertion. Should I print out the thing I want, the thing that failed or
Felix> both?
Typically, there are 3 items:
1) input data
2) action
3) output
4) expected output
Good failure should include all of the above in one way or another.
Felix>Than we could write "Assert.assertThat(adjustedDelay,
Felix> ToleranceMatcher.isAbout(1000L, 150L))" which could result in an error
Felix> message like
>
Felix> Expected: is about <1000L> with tolerance of <150L>
Felix> but was: <800L>
Would you be glad to receive bug report Bug 29562: Expected: is about
<1000L> with tolerance of <150L> but was: <800L> ?
Does the message include input data? No
Does it include action? No
Does it include output? Yes
Does it include expected output? Yes
All the items might be described in a human-understandable way
(especially in case of boolean-like results).
For instance: "When test duration is set to Long.MAX_VALUE, we don't
want the engine to cap or modify timer pauses, so the sleep delay of
1sec should be executed as ~1s"
Note: I have no idea the intention behind that test. It did took me a
while to browse the code back and forth to produce ANY meaningful
text, and I have absolutely no idea if my text has anything with
actual intention behind the test.
There might be similar test side by side with message like "When sleep
delay falls outside of the scheduled interval, we want to limit the
sleep to prevent threads waiting more than test duration"
Those kind of messages do include input data and action however those
are included in a free-form fashion.
Felix>Should I print out the thing I want, the thing that failed or
Felix> both?
For cases like "csv split test", or "regex test" or similar I like to
have function+its arguments as the message, so the failure looks like
AssertionError: split("1,2", delim=,) expected: {1, 2}, got: {1}
Just note: the message includes both input values, the action
("split"), and the output values.
One might think it is "duplication of the code" or that "it takes
enormous time to write the test", however:
1) The code is much more often read than written
2) For a given function (e.g. stringSplit) you create a single
function with assert call, then you use that checker to write trivial
looking tests:
checkSplit("1,2", 1, 2);
checkSplit("1", 1);
checkSplit("1,1,2", 1, 1, 2);
etc.
Of course there are Spock tests, there are @Parameterized, etc,
however my idea is even plain old JUnit 4 tests are pretty simple to
write and read.
By the way, as sebb said, test class and method name helps as well.
However Java method name with 200 chars and spaces does not sound
quite right to me :)
Felix> refactored and as stated above, the programmer has do good texting.
Undestandable code commends simplify refactoring, don't they?
Treat assert messages like code comments, except it is way better to
have the text in assert message rather than in a comment above.
The comment won't be visible in CI log
Felix> As always thoughtful and well put.
Thanks,
Vladimir
Re: Code review vs unit tests vs assert usage
Posted by Vladimir Sitnikov <si...@gmail.com>.
Philippe> We (except Vladimir :) ) all made some of those mistakes I
guess on JMeter
Philippe> and other projects.
Philippe> That’s how we become better devs.
Well, I learned that the hard way :)
Then I like sharing things (don't you remember I'm always looking for
performance-related or testing-related talks?), however I'm not sure I
can make a solid 1 hour long talk out of "assert messages".
Felix>4) review patches and give feedback :)
Philippe> PS: I still prefer improvable PR than no PR at all, improvable
Philippe> contribution than no contribution at all.
There's 5:
5) Refactor tests, and add clarifications that way you can learn the
codebase, and make useful contributions.
Vladimir
Re: Code review vs unit tests vs assert usage
Posted by Philippe Mouawad <ph...@gmail.com>.
@Vladimir, I reached the end of your mail, thanks for your insights, very
enriching !
We (except Vladimir :) ) all made some of those mistakes I guess on JMeter
and other projects.
That’s how we become better devs.
PS: I still prefer improvable PR than no PR at all, improvable
contribution than no contribution at all.
Regards
On Thursday, February 28, 2019, Felix Schumacher <
felix.schumacher@internetallee.de> wrote:
>
> Am 28.02.19 um 14:44 schrieb Vladimir Sitnikov:
>
>> Hi,
>>
>> I happen to maintain software, and I see a common pitfall when it comes to
>> unit tests.
>> Please don't take it personally, however please consider it when creating
>> new tests and/or when reviewing patches.
>>
>> I'm afraid the below might be too long, however below applies to JMeter
>> committers, to JMeter contributors, and it applies to any developers in
>> general.
>>
>> TL;DR:
>>
>> General rule: failing test should look like a good bug report. Thus
>> Assert.fail() is bad.
>>
> Agreed, at least without a meaningful description message.
>
>>
>> Consider using "single assertion" per test method. Having separate test
>> methods helps manual execution of the tests, and it makes test report
>> cleaner
>>
>> Consider using assertEquals(String message, expected, actual) instead of
>> assertTrue(expected == actual). The former allows you to provide human
>> readable message and it integrates well with IDEs (i.e. it allows to open
>> diff of expected and actual).
>>
>> If using just assertTrue(expected == actual) all you get is a stacktrace
>> and if such a test fails a developer has to reverse engineer the intention
>> behind that code.
>>
> +1
>
>>
>> == Problem statement ==
>>
>> Sample test code (it was added as a part of Bug 62785):
>>
>> SampleResult sample = sampler.sample();
>> assertFalse(sample.getResponseDataAsString().contains("java.io
>> .FileNotFoundException:"));
>>
>> Even though the test makes sure the code works, there's a maintenance
>> problem with this test.
>> When the test fails it is very cryptic.
>>
>> java.lang.AssertionError
>> at org.junit.Assert.fail(Assert.java:86)
>> at org.junit.Assert.assertTrue(Assert.java:41)
>> at org.junit.Assert.assertFalse(Assert.java:64)
>> at org.junit.Assert.assertFalse(Assert.java:74)
>> at
>> org.apache.jmeter.protocol.http.sampler.TestHTTPSamplers.tes
>> tRawBodyFromFile(TestHTTPSamplers.java:385)
>>
>> Of course we see that assertFalse was violated, however it is absolutely
>> obscure why the test failed and it is obscure what it was trying to do.
>>
>> == Desired behaviour ==
>>
>> Test failure should look like a good bug report.
>> For instance, if one can take a decent bug report by a simple copy&paste
>> "AssertionError" plus the stacktrace, then the assert is good.
>>
>> == Possible approaches ==
>>
>> Note: this all depends on a case-by-case basis.
>>
>> A) Use message parameter of assert* calls.
>> For instance: assertEquals("Response message should not contain
>> FileNotFoundException", false,
>> sample.getResponseDataAsString().contains("FileNotFoundException:"));
>>
>> Note it would still produce quite weird output like "expected false got
>> true".
>> In general, assertEquals for comparing booleans is almost always wrong.
>>
>> B) use if + assert.fail
>> if (sample.getResponseDataAsString().contains("FileNotFoundException:"))
>> {
>> Assert.fail("Response must not contain FNFE");
>> }
>>
>> C) Use Hamcrest matches. For instance:
>> assertThat(sample.getResponseDataAsString(),
>> not(containsString("java.io.FileNotFoundException:")));
>>
>> Of course "C" might be less verbose in this case, however there's
>> **extremely** important improvement which "C" brings over A and B (it can
>> be fixed though, but I made a deliberate mistake there).
>> ...
>> ...
>> Ok, you are great if you discovered that: assertThat print actual response
>> data in case the assert fails. In other words, we would know what was the
>> response.
>>
>
> I don't get what you are after here. Is this meant to be sarcasm?
>
> Your original version of "B" included the response string to make it more
> verbose and more tester friendly, so they are -- in my eyes -- rather
> equivalent in output.
>
> What I really like about "C" is that it is all code and a "standardized"
> output, which I expect a tester to be familiar with. I find myself often
> struggling to find the right words to state the expectation of the
> assertion. Should I print out the thing I want, the thing that failed or
> both?
>
>
>> D) Gradle + Spock might improve readability as well (see
>> https://twitter.com/VladimirSitnikv/status/1100666414548045825 )
>>
>> == Ok, I use message/assertThat, is it enough? ==
>> No. assertThat does not always help.
>> For instance, take a look into TimerServiceTest
>> https://github.com/apache/jmeter/blob/3d03af3d78746d67e7a81d
>> 2481e5e741b3907664/test/src/org/apache/jmeter/timers/Timer
>> ServiceTest.java#L34
>>
>> public void testBigInitialDelay() {
>> long now = System.currentTimeMillis();
>> long adjustedDelay = sut.adjustDelay(Long.MAX_VALUE, now + 1000L);
>> // As #adjustDelay uses System#currentTimeMillis we can't be sure, that
>> the value is exact 1000L
>> Assert.assertThat(Math.abs(adjustedDelay - 1000L) < 150L,
>> CoreMatchers.is(true));
>> }
>>
>> Does the test look good to you?
>> ...
>> ...
>> Felix?
>>
>
> No need to call me out :) I didn't like the test, when I wrote it, but at
> least I wrote it. It is a really good example of bad assertThat usage. I
> think it would be better to have Matcher that matches with a bit of
> tolerance. Than we could write "Assert.assertThat(adjustedDelay,
> ToleranceMatcher.isAbout(1000L, 150L))" which could result in an error
> message like
>
> Expected: is about <1000L> with tolerance of <150L>
> but was: <800L>
>
> ...
>> Ok, if you can spot the error, you are great.
>>
> The test will allow shorter delays than expected (but I thought that would
> be OK and not likely) and it probably has problems with really large
> negative delays. But I would be interested in the error you have found.
>
>>
>> The problem here is even though assertThat is used, test failure won't
>> explain the nature of the test. It won't explain WHY "true" was expected
>> and so on.
>>
>> Proper assert should be like the following:
>> if (...) { // Or Spock or whatever
>> Assert.fail("TimerService.sut.adjustDelay(Long.MAX_VALUE, now + 1000L)
>> should return something close to 1000 since now+1000 would anyway be less
>> than Long.MAX_VALUE. Actual result is " + adjustedDelay);
>> }
>>
>
> What I see here as a problem, is that you repeat values, that you used for
> testing previously. That is easily overlooked, when the test is refactored
> and as stated above, the programmer has do good texting.
>
> On the other hand, this message is nice.
>
>
>> == Next steps ==
>>
>> 0) The most common code smell is "computations in assert* messages".
>>
>> 1) "Constant" messages do not really help.
>> For instance, assertEquals("Incorrect entry returned",expected[i],
>> out[i]);
>> is a bad message.
>> It does not explain "why" something was expected. What was the input? What
>> was the action? Nothing of that is present in "incorrect entry returned"
>> message.
>>
>> 2) If you create a test assertion or an assert in the production code,
>> think how the failure look like.
>> You might even want to temporary put wrong values and see if the message
>> is
>> understandable and actionable.
>>
>> 3) If you review the patch, please pay attention on the way asserts are
>> written.
>> Note: reviewer often sees only positive side of the test. In other words,
>> CI says that tests pass. However reviewer does NOT see how the failure
>> would look like in CI.
>>
>
> +1
>
> 4) review patches and give feedback :)
>
>
>> Great applause to everybody who reach this line. Comments welcome.
>>
>
> Thanks for your thorough analysis and constructive feedback.
>
> As always thoughtful and well put.
>
> Felix
>
>
>> Vladimir
>>
>>
--
Cordialement.
Philippe Mouawad.
Re: Code review vs unit tests vs assert usage
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 28.02.19 um 14:44 schrieb Vladimir Sitnikov:
> Hi,
>
> I happen to maintain software, and I see a common pitfall when it comes to
> unit tests.
> Please don't take it personally, however please consider it when creating
> new tests and/or when reviewing patches.
>
> I'm afraid the below might be too long, however below applies to JMeter
> committers, to JMeter contributors, and it applies to any developers in
> general.
>
> TL;DR:
>
> General rule: failing test should look like a good bug report. Thus
> Assert.fail() is bad.
Agreed, at least without a meaningful description message.
>
> Consider using "single assertion" per test method. Having separate test
> methods helps manual execution of the tests, and it makes test report
> cleaner
>
> Consider using assertEquals(String message, expected, actual) instead of
> assertTrue(expected == actual). The former allows you to provide human
> readable message and it integrates well with IDEs (i.e. it allows to open
> diff of expected and actual).
>
> If using just assertTrue(expected == actual) all you get is a stacktrace
> and if such a test fails a developer has to reverse engineer the intention
> behind that code.
+1
>
> == Problem statement ==
>
> Sample test code (it was added as a part of Bug 62785):
>
> SampleResult sample = sampler.sample();
> assertFalse(sample.getResponseDataAsString().contains("java.io.FileNotFoundException:"));
>
> Even though the test makes sure the code works, there's a maintenance
> problem with this test.
> When the test fails it is very cryptic.
>
> java.lang.AssertionError
> at org.junit.Assert.fail(Assert.java:86)
> at org.junit.Assert.assertTrue(Assert.java:41)
> at org.junit.Assert.assertFalse(Assert.java:64)
> at org.junit.Assert.assertFalse(Assert.java:74)
> at
> org.apache.jmeter.protocol.http.sampler.TestHTTPSamplers.testRawBodyFromFile(TestHTTPSamplers.java:385)
>
> Of course we see that assertFalse was violated, however it is absolutely
> obscure why the test failed and it is obscure what it was trying to do.
>
> == Desired behaviour ==
>
> Test failure should look like a good bug report.
> For instance, if one can take a decent bug report by a simple copy&paste
> "AssertionError" plus the stacktrace, then the assert is good.
>
> == Possible approaches ==
>
> Note: this all depends on a case-by-case basis.
>
> A) Use message parameter of assert* calls.
> For instance: assertEquals("Response message should not contain
> FileNotFoundException", false,
> sample.getResponseDataAsString().contains("FileNotFoundException:"));
>
> Note it would still produce quite weird output like "expected false got
> true".
> In general, assertEquals for comparing booleans is almost always wrong.
>
> B) use if + assert.fail
> if (sample.getResponseDataAsString().contains("FileNotFoundException:")) {
> Assert.fail("Response must not contain FNFE");
> }
>
> C) Use Hamcrest matches. For instance:
> assertThat(sample.getResponseDataAsString(),
> not(containsString("java.io.FileNotFoundException:")));
>
> Of course "C" might be less verbose in this case, however there's
> **extremely** important improvement which "C" brings over A and B (it can
> be fixed though, but I made a deliberate mistake there).
> ...
> ...
> Ok, you are great if you discovered that: assertThat print actual response
> data in case the assert fails. In other words, we would know what was the
> response.
I don't get what you are after here. Is this meant to be sarcasm?
Your original version of "B" included the response string to make it
more verbose and more tester friendly, so they are -- in my eyes --
rather equivalent in output.
What I really like about "C" is that it is all code and a "standardized"
output, which I expect a tester to be familiar with. I find myself often
struggling to find the right words to state the expectation of the
assertion. Should I print out the thing I want, the thing that failed or
both?
>
> D) Gradle + Spock might improve readability as well (see
> https://twitter.com/VladimirSitnikv/status/1100666414548045825 )
>
> == Ok, I use message/assertThat, is it enough? ==
> No. assertThat does not always help.
> For instance, take a look into TimerServiceTest
> https://github.com/apache/jmeter/blob/3d03af3d78746d67e7a81d2481e5e741b3907664/test/src/org/apache/jmeter/timers/TimerServiceTest.java#L34
>
> public void testBigInitialDelay() {
> long now = System.currentTimeMillis();
> long adjustedDelay = sut.adjustDelay(Long.MAX_VALUE, now + 1000L);
> // As #adjustDelay uses System#currentTimeMillis we can't be sure, that
> the value is exact 1000L
> Assert.assertThat(Math.abs(adjustedDelay - 1000L) < 150L,
> CoreMatchers.is(true));
> }
>
> Does the test look good to you?
> ...
> ...
> Felix?
No need to call me out :) I didn't like the test, when I wrote it, but
at least I wrote it. It is a really good example of bad assertThat
usage. I think it would be better to have Matcher that matches with a
bit of tolerance. Than we could write "Assert.assertThat(adjustedDelay,
ToleranceMatcher.isAbout(1000L, 150L))" which could result in an error
message like
Expected: is about <1000L> with tolerance of <150L>
but was: <800L>
> ...
> Ok, if you can spot the error, you are great.
The test will allow shorter delays than expected (but I thought that
would be OK and not likely) and it probably has problems with really
large negative delays. But I would be interested in the error you have
found.
>
> The problem here is even though assertThat is used, test failure won't
> explain the nature of the test. It won't explain WHY "true" was expected
> and so on.
>
> Proper assert should be like the following:
> if (...) { // Or Spock or whatever
> Assert.fail("TimerService.sut.adjustDelay(Long.MAX_VALUE, now + 1000L)
> should return something close to 1000 since now+1000 would anyway be less
> than Long.MAX_VALUE. Actual result is " + adjustedDelay);
> }
What I see here as a problem, is that you repeat values, that you used
for testing previously. That is easily overlooked, when the test is
refactored and as stated above, the programmer has do good texting.
On the other hand, this message is nice.
>
> == Next steps ==
>
> 0) The most common code smell is "computations in assert* messages".
>
> 1) "Constant" messages do not really help.
> For instance, assertEquals("Incorrect entry returned",expected[i],
> out[i]);
> is a bad message.
> It does not explain "why" something was expected. What was the input? What
> was the action? Nothing of that is present in "incorrect entry returned"
> message.
>
> 2) If you create a test assertion or an assert in the production code,
> think how the failure look like.
> You might even want to temporary put wrong values and see if the
> message is
> understandable and actionable.
>
> 3) If you review the patch, please pay attention on the way asserts are
> written.
> Note: reviewer often sees only positive side of the test. In other words,
> CI says that tests pass. However reviewer does NOT see how the failure
> would look like in CI.
+1
4) review patches and give feedback :)
>
> Great applause to everybody who reach this line. Comments welcome.
Thanks for your thorough analysis and constructive feedback.
As always thoughtful and well put.
Felix
>
> Vladimir
>