Lucene - Core
  1. Lucene - Core
  2. LUCENE-3506

tests for verifying that assertions are enabled do nothing since they ignore AssertionError

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: general/test
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Follow-up from LUCENE-3501

      1. LUCENE-3506.patch
        6 kB
        Doron Cohen
      2. LUCENE-3506.patch
        5 kB
        Doron Cohen

        Activity

        Hide
        Doron Cohen added a comment -

        Attached fix for this:

        • assertionsEnabled() method added to LTC.
        • tests that were no op were fixed to actually test that the assertion failed.
        • after the fix, in trunk, analyzer's final'ness assertion tests failed because being final (class or method) is no longer needed in trunk. So these tests were removed in TestAssertions.
          • note: should not remove these tests when merging to 3x.
        • TestSegmentMerger also failed with this fix - because it used the stale IW's SegmentInfos to create a compound segment. Fixed by reading a fresh SIS.
        • only one test (TestAssertions.testbasics()) fails if assertions are not enabled. The other tests do not fail (though they do execute). I think that this was intended in the code, thought since it did not work it is hard to tell...

        This is ready to commit.

        Show
        Doron Cohen added a comment - Attached fix for this: assertionsEnabled() method added to LTC. tests that were no op were fixed to actually test that the assertion failed. after the fix, in trunk, analyzer's final'ness assertion tests failed because being final (class or method) is no longer needed in trunk. So these tests were removed in TestAssertions. note: should not remove these tests when merging to 3x. TestSegmentMerger also failed with this fix - because it used the stale IW's SegmentInfos to create a compound segment. Fixed by reading a fresh SIS. only one test (TestAssertions.testbasics()) fails if assertions are not enabled. The other tests do not fail (though they do execute). I think that this was intended in the code, thought since it did not work it is hard to tell... This is ready to commit.
        Hide
        Chris Male added a comment -

        Thanks for catching the Analyzer final assertion test issue.

        Show
        Chris Male added a comment - Thanks for catching the Analyzer final assertion test issue.
        Hide
        Uwe Schindler added a comment -

        +1 to commit!

        Show
        Uwe Schindler added a comment - +1 to commit!
        Hide
        Michael McCandless added a comment -

        I'm confused here – the changes to TestSegmentMerger look like they'll allow the test to pass when assertions are disabled?

        (Whereas today if you run that test w/o assertions you get a failure, albeit a confusing one).

        Show
        Michael McCandless added a comment - I'm confused here – the changes to TestSegmentMerger look like they'll allow the test to pass when assertions are disabled? (Whereas today if you run that test w/o assertions you get a failure, albeit a confusing one).
        Hide
        Robert Muir added a comment -

        maybe make assertionsEnabled() static in LuceneTestCase, and add assertTrue(assertionsEnabled()) in LuceneTestCase's beforeClass?

        This way all tests will fail if assertions are not enabled.

        The other day I committed an accidental change to common-build that disabled assertions, and it was a little confusing to track down.

        Show
        Robert Muir added a comment - maybe make assertionsEnabled() static in LuceneTestCase, and add assertTrue(assertionsEnabled()) in LuceneTestCase's beforeClass? This way all tests will fail if assertions are not enabled. The other day I committed an accidental change to common-build that disabled assertions, and it was a little confusing to track down.
        Hide
        Doron Cohen added a comment -

        (Whereas today if you run that test w/o assertions you get a failure, albeit a confusing one).

        Actually today when you run the tests - with assertions, without assertions, - you get no failures at all - which is what I was trying to fix here (unless I missed something seriously) - because:

        • the original tests, after deciding to fail, invoked fail()
        • this threw AssertionError
        • but it was ignored as part of their wrong logic.

        I'm confused here – the changes to TestSegmentMerger look like they'll allow the test to pass when assertions are disabled?

        Right, I fixed it such that only if assertions are enabled, they verify that the expected assertion errors are not thrown, so they allow you to run tests also without enabling assertions. See my comment above "only one test...". I take it that this kind of flexibility is not required. So will change it so that these tests will fail if assertions are not enabled.

        The other day I committed an accidental change to common-build that disabled assertions, and it was a little confusing to track down.

        I see, so we make the entire test framework to fail if assertions are not enabled.
        I'll update the patch.

        Show
        Doron Cohen added a comment - (Whereas today if you run that test w/o assertions you get a failure, albeit a confusing one). Actually today when you run the tests - with assertions, without assertions, - you get no failures at all - which is what I was trying to fix here (unless I missed something seriously) - because: the original tests, after deciding to fail, invoked fail() this threw AssertionError but it was ignored as part of their wrong logic. I'm confused here – the changes to TestSegmentMerger look like they'll allow the test to pass when assertions are disabled? Right, I fixed it such that only if assertions are enabled, they verify that the expected assertion errors are not thrown, so they allow you to run tests also without enabling assertions. See my comment above "only one test...". I take it that this kind of flexibility is not required. So will change it so that these tests will fail if assertions are not enabled. The other day I committed an accidental change to common-build that disabled assertions, and it was a little confusing to track down. I see, so we make the entire test framework to fail if assertions are not enabled. I'll update the patch.
        Hide
        Doron Cohen added a comment -

        Updated patch as suggested, thanks guys for reviewing and your helpful comments.

        Show
        Doron Cohen added a comment - Updated patch as suggested, thanks guys for reviewing and your helpful comments.
        Hide
        Michael McCandless added a comment -

        Actually today when you run the tests - with assertions, without assertions, - you get no failures at all - which is what I was trying to fix here (unless I missed something seriously) - because:

        • the original tests, after deciding to fail, invoked fail()
        • this threw AssertionError
        • but it was ignored as part of their wrong logic.

        Ahh, OK, I missed that fail() throws AssertionError which this then caught & ignored. OK. Patch looks good!

        Show
        Michael McCandless added a comment - Actually today when you run the tests - with assertions, without assertions, - you get no failures at all - which is what I was trying to fix here (unless I missed something seriously) - because: the original tests, after deciding to fail, invoked fail() this threw AssertionError but it was ignored as part of their wrong logic. Ahh, OK, I missed that fail() throws AssertionError which this then caught & ignored. OK. Patch looks good!
        Hide
        Doron Cohen added a comment -

        Fixed:

        • r1188777 - trunk
        • r1188801 - 3x

        Mike, Uwe, Robert, thanks for reviewing!

        Show
        Doron Cohen added a comment - Fixed: r1188777 - trunk r1188801 - 3x Mike, Uwe, Robert, thanks for reviewing!
        Hide
        Dawid Weiss added a comment -

        Err... how is this different:

        assert Boolean.FALSE.booleanValue();
        

        from

        assert false;
        

        Is there any compile-time code elimination? I ask specifically because I've implemented a dedicated validator for this purpose in RandomizedRunner here:

        https://github.com/carrotsearch/randomizedtesting/blob/master/runner/src/main/java/com/carrotsearch/randomizedtesting/validators/EnsureAssertionsEnabled.java

        and this seems to work just fine (checked with and without -ea).

        Show
        Dawid Weiss added a comment - Err... how is this different: assert Boolean .FALSE.booleanValue(); from assert false ; Is there any compile-time code elimination? I ask specifically because I've implemented a dedicated validator for this purpose in RandomizedRunner here: https://github.com/carrotsearch/randomizedtesting/blob/master/runner/src/main/java/com/carrotsearch/randomizedtesting/validators/EnsureAssertionsEnabled.java and this seems to work just fine (checked with and without -ea).
        Hide
        Uwe Schindler added a comment -

        It may no longer apply to Java 6 javac, but when I implemented this test for the first time (was Java 4 or Java 5) the "assert false;" made javac angry - it simply refused to compile this (some error like unreachable statement blabla). This was the only way to get this running, copied from some code example on the net (I think it was the assertion guide shipped with Java 1.4).

        Show
        Uwe Schindler added a comment - It may no longer apply to Java 6 javac, but when I implemented this test for the first time (was Java 4 or Java 5) the "assert false;" made javac angry - it simply refused to compile this (some error like unreachable statement blabla). This was the only way to get this running, copied from some code example on the net (I think it was the assertion guide shipped with Java 1.4).
        Hide
        Yonik Seeley added a comment -

        Tests run from intellij now fail saying that assertions are not enabled. Anyone know the right way to change "ant idea" so that assertions are enabled by default?

        Also, we've often done performance tests as unit tests in the past. Is there an easy way to disable this "assertions enabled" test?

        Show
        Yonik Seeley added a comment - Tests run from intellij now fail saying that assertions are not enabled. Anyone know the right way to change "ant idea" so that assertions are enabled by default? Also, we've often done performance tests as unit tests in the past. Is there an easy way to disable this "assertions enabled" test?
        Hide
        Steve Rowe added a comment -

        Tests run from intellij now fail saying that assertions are not enabled. Anyone know the right way to change "ant idea" so that assertions are enabled by default?

        All of the pre-defined run configurations (one for each module) enable assertions via the "-ea" cmdline param. Not sure how to globally enable assertions in IntelliJ.

        Show
        Steve Rowe added a comment - Tests run from intellij now fail saying that assertions are not enabled. Anyone know the right way to change "ant idea" so that assertions are enabled by default? All of the pre-defined run configurations (one for each module) enable assertions via the "-ea" cmdline param. Not sure how to globally enable assertions in IntelliJ.
        Hide
        Erik Hatcher added a comment -

        IJ has a default setup for "JUnit" execution, and putting in VM arg of "-ea" should do the trick.

        Show
        Erik Hatcher added a comment - IJ has a default setup for "JUnit" execution, and putting in VM arg of "-ea" should do the trick.
        Hide
        Dawid Weiss added a comment -

        You can also enable assertions just for the class/package which checks if assertions are enabled, Yonik. This should make the check pass and disable all other assertions (for benchmarking). I don't remember the syntax off the top of my head though.

        Show
        Dawid Weiss added a comment - You can also enable assertions just for the class/package which checks if assertions are enabled, Yonik. This should make the check pass and disable all other assertions (for benchmarking). I don't remember the syntax off the top of my head though.
        Hide
        Steve Rowe added a comment -

        IJ has a default setup for "JUnit" execution, and putting in VM arg of "-ea" should do the trick.

        Thanks, Erik - I just committed this change to the IntelliJ IDEA configuration under dev-tools/:

        • r1189527: trunk
        • r1189529: branch_3x
        Show
        Steve Rowe added a comment - IJ has a default setup for "JUnit" execution, and putting in VM arg of "-ea" should do the trick. Thanks, Erik - I just committed this change to the IntelliJ IDEA configuration under dev-tools/ : r1189527: trunk r1189529: branch_3x
        Hide
        Doron Cohen added a comment -

        I just committed this change to the IntelliJ IDEA configuration

        Thanks for fixing for IntelliJ!

        Show
        Doron Cohen added a comment - I just committed this change to the IntelliJ IDEA configuration Thanks for fixing for IntelliJ!
        Hide
        Doron Cohen added a comment -

        Also, we've often done performance tests as unit tests in the past. Is there an easy way to disable this "assertions enabled" test?

        You can also enable assertions just for the class/package which checks if assertions are enabled, Yonik. This should make the check pass and disable all other assertions (for benchmarking). I don't remember the syntax off the top of my head though.

        Yonik, is this sufficient for running the perf tests?
        Otherwise I can add a -D flag for disabling testing this in LTC.

        Show
        Doron Cohen added a comment - Also, we've often done performance tests as unit tests in the past. Is there an easy way to disable this "assertions enabled" test? You can also enable assertions just for the class/package which checks if assertions are enabled, Yonik. This should make the check pass and disable all other assertions (for benchmarking). I don't remember the syntax off the top of my head though. Yonik, is this sufficient for running the perf tests? Otherwise I can add a -D flag for disabling testing this in LTC.
        Hide
        Doron Cohen added a comment -

        For easier perf testing I added a -D flag to tell LTC not to fail each and every test if Java assertions are not enabled:

        -Dtests.asserts.gracious=true
        

        (Tests requiring Java assertions - e.g. TestAssertions - will still fail, on purpose.)

        • r1189655 - trunk
        • r1189663 - 3x
        Show
        Doron Cohen added a comment - For easier perf testing I added a -D flag to tell LTC not to fail each and every test if Java assertions are not enabled: -Dtests.asserts.gracious=true (Tests requiring Java assertions - e.g. TestAssertions - will still fail, on purpose.) r1189655 - trunk r1189663 - 3x
        Hide
        Yonik Seeley added a comment -

        Thanks Doron, that works!

        Show
        Yonik Seeley added a comment - Thanks Doron, that works!

          People

          • Assignee:
            Doron Cohen
            Reporter:
            Doron Cohen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development