@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 < [email protected]> 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.
