If you want to always run it and not rely on assert why not just put
it in an if (cond) { Assert.fail(...); }? This seems to strike the
balance between what you both need?> Hmm, ok -- i thought this was happening when the JVM was forked. The test runner doesn't mess with assertions at all, they're configured as regular JVM parameters at the configuration level: https://github.com/apache/lucene-solr/blob/trunk/lucene/common-build.xml#L139-L141 https://github.com/apache/lucene-solr/blob/trunk/lucene/common-build.xml#L997 What you have in Lucene (tests.asserts): https://github.com/apache/lucene-solr/blob/trunk/lucene/common-build.xml#L1034-L1035 is to pass the information whether we should run without assertions because, by default, we require them when running tests: https://github.com/apache/lucene-solr/blob/trunk/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleAssertionsRequired.java#L34-L52 Dawid On Thu, Aug 27, 2015 at 2:01 AM, Chris Hostetter <[email protected]> wrote: > > : 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] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
