drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Barclay <dbarc...@maprtech.com>
Subject Re: Please don't use assert in unit tests
Date Thu, 28 May 2015 20:21:27 GMT
 > the test ... actually checked for AssertionError


We really need to think about what methods are supposed to do (the calling
contract), not what they currently do.


Chris Westin wrote:
> In this context, I don't see a difference between test code, and test setup
> code. If you run the test without assertions enabled, and it appears to
> pass as a result, you've been misled (this is what got me started down this
> path). Setup failure may amount to the test doing nothing, not running, or
> only partially testing what it was meant to.
> Quite simply: the result of running the test should be the same whether
> Java assertions are enabled or not. "result" here includes not just any
> textual output, but execution of what the test actually does.
> Note that this may imply that some non-test code should also not be using
> Java assert. For example, one problem I ran into was that nullable value
> vectors' implementation of get() used "assert !isNull()". This in turn led
> to the test (in TestValueVector) actually checked for AssertionError to be
> thrown, but only if assertions were enabled. In this case, the test failed
> if assertions were NOT enabled. I fixed ValueVectors (in a previous patch)
> to throw an IllegalArgumentException if you try to get a value that is null
> -- something which could have also misled users badly by returning an
> uninitialized value for something that should have been null, if there were
> a bug elsewhere in the code that didn't check for null first.
> I'm beginning to think we should go the Google route and not allow Java
> assert anywhere, instead always throwing real exceptions (possibly
> indirectly by using the Preconditions package). But I'm not quite ready to
> go there yet. For now I'll stick to asking that we not use Java assert in
> tests and test support code.
> On Thu, May 28, 2015 at 11:12 AM, Jason Altekruse <altekrusejason@gmail.com>
> wrote:
>> 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 <chriswestin42@gmail.com>
>> 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

Daniel Barclay
MapR Technologies

View raw message