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
