Hello Drillers, I merged a patch from Chris yesterday that cleaned up and fixed some issues in a good number of the unit tests.
The primary problem he solved was improper usage of Java asserts to test for the detecting success conditions in the tests. There was no single contributor or component that they were added by (several of them were from my own tests), so I would just like to remind everyone that there should be no expectation that assertions are enabled for proper functionality, even in the unit tests. As is now the case throughout the code, please use JUnit assert functions (assertTrue, assertEquals, etc.) for testing success/failure conditions in tests. While I did review the patch before I merged it, Daniel noticed that some of the asserts that were refactored were not actually designed for checking Drill code/functionality, but had been added to indicate improper test setup. In these cases he had deliberately added Java asserts to differentiate these two cases. He is fine if we want to ban Java asserts from tests, but would like to have a discussion about possible ways to differentiate checks for test setup correctness from actual test failures. The two options I can think of are allowing java asserts back into the unit tests, but only to test issues with test setup. The other is to add a standard around messages to differentiate these two cases if we use the Junit assert methods for both of these cases. Please respond with your objections or approval of either of these methods. Once we have a good consensus I will be adding this to the contribution guide on the drill docs. On Thu, Apr 30, 2015 at 7:50 PM, Chris Westin <[email protected]> wrote: > I had some confusing times this afternoon as I was working through unit > tests to validate the new allocator I've been working on. (Yes, > TopLevelAllocator will be replaced soon; and the new allocator found leaks > that were happening before.) > > By default, when running a junit test in my IDE (eclipse, but I'm sure this > is true elsewhere), it gets no JVM arguments. It's possible to create a > test profile that includes JVM arguments, and this is what is required if > you want to supply things like -ea (enable assertions in the JVM). > > This afternoon I was fooled by some tests that appeared to work when I ran > them. This was because they checked their results with java assert, instead > of with the org.junit.Assert.assertXXX static functions, and I didn't have > -ea. (Yes, I've fixed this in my current branch, and replaced the asserts > with the appropriate junit assertXXX()s.) > > So, don't use assert to check test results (or verify state in helpers) in > unit tests. This can badly mislead someone who's running the tests into > thinking they are working even if they aren't. > > And BTW, I also spotted some tests that were using the junit's assertEquals > like this: assertEquals(true, <boolean-condition>). Just FYI, for things > like this, you can use assertTrue(<boolean-condition>). There's also an > assertFalse(). Check the javadoc if you need something beyond > assertEquals() -- there are other options. > > "Thank you for observing all safety precautions." > Chris >
