On Thu, 28 Feb 2019 at 13:44, Vladimir Sitnikov
<[email protected]> 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
