> 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 <[email protected]>
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 <[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
--
Daniel Barclay
MapR Technologies
