: 1) The test framework never randomly disables asserts (it cannot do : this, because asserts are enabled before the VM starts - you cannot do
Hmm, ok -- i thought this was happening when the JVM was forked. In any case, my underlying concern still seems valid to me: confusing nonsensical test failures could occur if someone or some jenkins runs with -Dtests.asserts=false (which we should really do in at least one jenkins job to ensure we're not relying on side effects of method called in an assert ... wasn't that the whole point of adding tests.asserts in LUCENE-6019?) : (we also have a test that validates this). In addition, our tests should : not check the JVM for correctness, it is just there to make an : assumption about how WS should behave. So when this assert fails, the : test is still not wrong. i think having sanity checks about our assumptions of the JVM are useful -- just like we (last time i checked) have sanity checks that filesystems behave as we expect. It's one thing to say "we don't need a check that 2 + 2 == 4" but in cases like this where assumptions (about hte definition of whitespace) can evidently change between JVM versions, it's nice to have a sanity check that our tests are testing the right thing (ie: fail fast because the JVM isn't behaving the way we expect, don't fail slow with a weird obscure NPE or AIOBE error because our code expected 2+2==4 and the JVM gave us -42 instead) : 2) Now the answer: I changed it because of performance. The Ah, ok -- totally fair point. Good call. : Based on (1) and (2) this is OK. If you still want to revert to : Assert.assertTrue like before, please use an if-condition instead: : if (not whitespace) Assert.fail(String.format(error message)); Actaully, the more i think about it, the more i want to: a) add an explicit test that loops over this array and fails with a clear "your JVM is weird and doesn't consider thi char whitespace, that's messed up and this test and possible some other code needs updated if this is the new Java/unicode rules" b) leave the plain assert you have in place as a fallback to provide a clera assertion msg in case someone runs a test method/class that uses this utility method but don't run the sanity check test mentioned in (a) because of -Dtestcase or -Dtest.method. (yes, i realize only one test method currently uses it nad the utility has already been refactored up to live in that same test class, but this is all about future proofing in case it ever gets refactred again) -Hoss http://www.lucidworks.com/ --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
