[ https://issues.apache.org/jira/browse/LUCENE-8999?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Chris M. Hostetter reassigned LUCENE-8999: ------------------------------------------ Fix Version/s: 8.3 master (9.0) Affects Version/s: 8.2 Assignee: Chris M. Hostetter Description: Up to Lucene 8.2, if {{expectThrows}} (or one of it's variants) was given a Runnable that contained an assert/assume call which failed somehwere down it's stack, it would catch these and re-wrap them in a new assertion failure. (unless it matched the "expected" exception type). This would mean that tests which should have been SKIPed due to a bad assumption about the local ENV would instead FAIL. This issue tracks a change such that {{expectThrow}} now directly re-throws any instances of {{AssertionError}} or {{AssumptionViolatedException}} _unless_ they are instances of the expected exception type specified by the user. Original jira summary below... ---- Once upon a time, {{TestRunWithRestrictedPermissions}} use to have test methods that looked like this... {code:java} try { runWithRestrictedPermissions(this::doSomeForbiddenStuff); fail("this should not pass!"); } catch (SecurityException se) { // pass } {code} LUCENE-8938 changed this code to look like this... {code:java} expectThrows(SecurityException.class, () -> runWithRestrictedPermissions(this::doSomeForbiddenStuff)); {code} But a nuance of the existing code that isn't captured in the new code is that {{runWithRestrictedPermissions(...)}} explicitly uses {{assumeTrue(..., System.getSecurityManager() != null)}} to ensure that if a security manager is not in use, the test should be {{SKIPed}} and not considered a pass or a fail. The key issue being that {{assumeTrue(...)}} (and other 'assume' related methods like it) throws an {{AssumptionViolatedException}} when the condition isn't met, expecting this to propagate up to the Test Runner. With the _old_ code this worked as expected - the {{AssumptionViolatedException}} would abort execution before the {{fail(...)}} but not be caught by the {{catch}} and bubble up all the way to the test runner so the test would be recorded as a SKIP. With the new code, {{expectThrows()}} is catching the {{AssumptionViolatedException}} and since it doesn't match the expected {{SecurityException.class}} is generating a test failure instead... {noformat} [junit4] Suite: org.apache.lucene.util.TestRunWithRestrictedPermissions [junit4] 2> NOTE: download the large Jenkins line-docs file by running 'ant get-jenkins-line-docs' in the lucene directory. [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestRunWithRestrictedPermissions -Dtests.method=testCompletelyForbidden2 -Dtests.seed=4181E5FE9E84DBC4 -Dtests.multiplier=2 -Dtests.nightly=true -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=luy -Dtests.timezone=Etc/GMT-7 -Dtests.asserts=true -Dtests.file.encoding=US-ASCII [junit4] FAILURE 0.10s J7 | TestRunWithRestrictedPermissions.testCompletelyForbidden2 <<< [junit4] > Throwable #1: junit.framework.AssertionFailedError: Unexpected exception type, expected SecurityException but got org.junit.AssumptionViolatedException: runWithRestrictedPermissions requires a SecurityManager enabled [junit4] > at __randomizedtesting.SeedInfo.seed([4181E5FE9E84DBC4:16509163A0E04B41]:0) [junit4] > at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2729) [junit4] > at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2718) [junit4] > at org.apache.lucene.util.TestRunWithRestrictedPermissions.testCompletelyForbidden2(TestRunWithRestrictedPermissions.java:39) [junit4] > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [junit4] > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) [junit4] > at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) [junit4] > at java.base/java.lang.reflect.Method.invoke(Method.java:566) [junit4] > at java.base/java.lang.Thread.run(Thread.java:834) [junit4] > Caused by: org.junit.AssumptionViolatedException: runWithRestrictedPermissions requires a SecurityManager enabled [junit4] > at com.carrotsearch.randomizedtesting.RandomizedTest.assumeTrue(RandomizedTest.java:725) [junit4] > at org.apache.lucene.util.LuceneTestCase.assumeTrue(LuceneTestCase.java:873) [junit4] > at org.apache.lucene.util.LuceneTestCase.runWithRestrictedPermissions(LuceneTestCase.java:2917) [junit4] > at org.apache.lucene.util.TestRunWithRestrictedPermissions.lambda$testCompletelyForbidden2$2(TestRunWithRestrictedPermissions.java:40) [junit4] > at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2724) [junit4] > ... 37 more {noformat} ---- While there might be easy fixes that could be made explicitly to {{TestRunWithRestrictedPermissions}} to deal with this particular problem, it seems like perhaps we should consider changes to better deal with this _type_ of problem that might exist elsewhere or occur in the future? was: Once upon a time, {{TestRunWithRestrictedPermissions}} use to have test methods that looked like this... {code:java} try { runWithRestrictedPermissions(this::doSomeForbiddenStuff); fail("this should not pass!"); } catch (SecurityException se) { // pass } {code} LUCENE-8938 changed this code to look like this... {code:java} expectThrows(SecurityException.class, () -> runWithRestrictedPermissions(this::doSomeForbiddenStuff)); {code} But a nuance of the existing code that isn't captured in the new code is that {{runWithRestrictedPermissions(...)}} explicitly uses {{assumeTrue(..., System.getSecurityManager() != null)}} to ensure that if a security manager is not in use, the test should be {{SKIPed}} and not considered a pass or a fail. The key issue being that {{assumeTrue(...)}} (and other 'assume' related methods like it) throws an {{AssumptionViolatedException}} when the condition isn't met, expecting this to propagate up to the Test Runner. With the _old_ code this worked as expected - the {{AssumptionViolatedException}} would abort execution before the {{fail(...)}} but not be caught by the {{catch}} and bubble up all the way to the test runner so the test would be recorded as a SKIP. With the new code, {{expectThrows()}} is catching the {{AssumptionViolatedException}} and since it doesn't match the expected {{SecurityException.class}} is generating a test failure instead... {noformat} [junit4] Suite: org.apache.lucene.util.TestRunWithRestrictedPermissions [junit4] 2> NOTE: download the large Jenkins line-docs file by running 'ant get-jenkins-line-docs' in the lucene directory. [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestRunWithRestrictedPermissions -Dtests.method=testCompletelyForbidden2 -Dtests.seed=4181E5FE9E84DBC4 -Dtests.multiplier=2 -Dtests.nightly=true -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=luy -Dtests.timezone=Etc/GMT-7 -Dtests.asserts=true -Dtests.file.encoding=US-ASCII [junit4] FAILURE 0.10s J7 | TestRunWithRestrictedPermissions.testCompletelyForbidden2 <<< [junit4] > Throwable #1: junit.framework.AssertionFailedError: Unexpected exception type, expected SecurityException but got org.junit.AssumptionViolatedException: runWithRestrictedPermissions requires a SecurityManager enabled [junit4] > at __randomizedtesting.SeedInfo.seed([4181E5FE9E84DBC4:16509163A0E04B41]:0) [junit4] > at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2729) [junit4] > at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2718) [junit4] > at org.apache.lucene.util.TestRunWithRestrictedPermissions.testCompletelyForbidden2(TestRunWithRestrictedPermissions.java:39) [junit4] > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [junit4] > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) [junit4] > at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) [junit4] > at java.base/java.lang.reflect.Method.invoke(Method.java:566) [junit4] > at java.base/java.lang.Thread.run(Thread.java:834) [junit4] > Caused by: org.junit.AssumptionViolatedException: runWithRestrictedPermissions requires a SecurityManager enabled [junit4] > at com.carrotsearch.randomizedtesting.RandomizedTest.assumeTrue(RandomizedTest.java:725) [junit4] > at org.apache.lucene.util.LuceneTestCase.assumeTrue(LuceneTestCase.java:873) [junit4] > at org.apache.lucene.util.LuceneTestCase.runWithRestrictedPermissions(LuceneTestCase.java:2917) [junit4] > at org.apache.lucene.util.TestRunWithRestrictedPermissions.lambda$testCompletelyForbidden2$2(TestRunWithRestrictedPermissions.java:40) [junit4] > at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2724) [junit4] > ... 37 more {noformat} ---- While there might be easy fixes that could be made explicitly to {{TestRunWithRestrictedPermissions}} to deal with this particular problem, it seems like perhaps we should consider changes to better deal with this _type_ of problem that might exist elsewhere or occur in the future? > expectThrows doesn't play nicely with "assume" failures > ------------------------------------------------------- > > Key: LUCENE-8999 > URL: https://issues.apache.org/jira/browse/LUCENE-8999 > Project: Lucene - Core > Issue Type: Test > Affects Versions: 8.2 > Reporter: Chris M. Hostetter > Assignee: Chris M. Hostetter > Priority: Major > Fix For: master (9.0), 8.3 > > Attachments: LUCENE-8999.patch > > > Up to Lucene 8.2, if {{expectThrows}} (or one of it's variants) was given a > Runnable that contained an assert/assume call which failed somehwere down > it's stack, it would catch these and re-wrap them in a new assertion failure. > (unless it matched the "expected" exception type). This would mean that > tests which should have been SKIPed due to a bad assumption about the local > ENV would instead FAIL. > This issue tracks a change such that {{expectThrow}} now directly re-throws > any instances of {{AssertionError}} or {{AssumptionViolatedException}} > _unless_ they are instances of the expected exception type specified by the > user. > Original jira summary below... > ---- > Once upon a time, {{TestRunWithRestrictedPermissions}} use to have test > methods that looked like this... > {code:java} > try { > runWithRestrictedPermissions(this::doSomeForbiddenStuff); > fail("this should not pass!"); > } catch (SecurityException se) { > // pass > } > {code} > LUCENE-8938 changed this code to look like this... > {code:java} > expectThrows(SecurityException.class, () -> > runWithRestrictedPermissions(this::doSomeForbiddenStuff)); > {code} > But a nuance of the existing code that isn't captured in the new code is that > {{runWithRestrictedPermissions(...)}} explicitly uses {{assumeTrue(..., > System.getSecurityManager() != null)}} to ensure that if a security manager > is not in use, the test should be {{SKIPed}} and not considered a pass or a > fail. > The key issue being that {{assumeTrue(...)}} (and other 'assume' related > methods like it) throws an {{AssumptionViolatedException}} when the condition > isn't met, expecting this to propagate up to the Test Runner. > With the _old_ code this worked as expected - the > {{AssumptionViolatedException}} would abort execution before the > {{fail(...)}} but not be caught by the {{catch}} and bubble up all the way to > the test runner so the test would be recorded as a SKIP. > With the new code, {{expectThrows()}} is catching the > {{AssumptionViolatedException}} and since it doesn't match the expected > {{SecurityException.class}} is generating a test failure instead... > {noformat} > [junit4] Suite: org.apache.lucene.util.TestRunWithRestrictedPermissions > [junit4] 2> NOTE: download the large Jenkins line-docs file by running > 'ant get-jenkins-line-docs' in the lucene directory. > [junit4] 2> NOTE: reproduce with: ant test > -Dtestcase=TestRunWithRestrictedPermissions > -Dtests.method=testCompletelyForbidden2 -Dtests.seed=4181E5FE9E84DBC4 > -Dtests.multiplier=2 -Dtests.nightly=true -Dtests.slow=true > -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt > -Dtests.locale=luy -Dtests.timezone=Etc/GMT-7 -Dtests.asserts=true > -Dtests.file.encoding=US-ASCII > [junit4] FAILURE 0.10s J7 | > TestRunWithRestrictedPermissions.testCompletelyForbidden2 <<< > [junit4] > Throwable #1: junit.framework.AssertionFailedError: > Unexpected exception type, expected SecurityException but got > org.junit.AssumptionViolatedException: runWithRestrictedPermissions requires > a SecurityManager enabled > [junit4] > at > __randomizedtesting.SeedInfo.seed([4181E5FE9E84DBC4:16509163A0E04B41]:0) > [junit4] > at > org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2729) > [junit4] > at > org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2718) > [junit4] > at > org.apache.lucene.util.TestRunWithRestrictedPermissions.testCompletelyForbidden2(TestRunWithRestrictedPermissions.java:39) > [junit4] > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > [junit4] > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > [junit4] > at > java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > [junit4] > at > java.base/java.lang.reflect.Method.invoke(Method.java:566) > [junit4] > at java.base/java.lang.Thread.run(Thread.java:834) > [junit4] > Caused by: org.junit.AssumptionViolatedException: > runWithRestrictedPermissions requires a SecurityManager enabled > [junit4] > at > com.carrotsearch.randomizedtesting.RandomizedTest.assumeTrue(RandomizedTest.java:725) > [junit4] > at > org.apache.lucene.util.LuceneTestCase.assumeTrue(LuceneTestCase.java:873) > [junit4] > at > org.apache.lucene.util.LuceneTestCase.runWithRestrictedPermissions(LuceneTestCase.java:2917) > [junit4] > at > org.apache.lucene.util.TestRunWithRestrictedPermissions.lambda$testCompletelyForbidden2$2(TestRunWithRestrictedPermissions.java:40) > [junit4] > at > org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2724) > [junit4] > ... 37 more > {noformat} > ---- > While there might be easy fixes that could be made explicitly to > {{TestRunWithRestrictedPermissions}} to deal with this particular problem, it > seems like perhaps we should consider changes to better deal with this _type_ > of problem that might exist elsewhere or occur in the future? -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org