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

Ack!


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

Daniel




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

Mime
View raw message