expectThrows doesn't play nicely with "assume" failures [LUCENE-8999]

mikemccand commented 4 years ago

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...

try {
  runWithRestrictedPermissions(this::doSomeForbiddenStuff);
  fail("this should not pass!");
} catch (SecurityException se) {
  // pass
}

LUCENE-8938 changed this code to look like this...

expectThrows(SecurityException.class, () -> runWithRestrictedPermissions(this::doSomeForbiddenStuff));

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...

   [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

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?

Legacy Jira details

LUCENE-8999 by Chris M. Hostetter on Oct 03 2019, resolved Oct 07 2019 Attachments: LUCENE-8999.patch Linked issues:

LUCENE-8938

mikemccand commented 4 years ago

strawman: should expectThrows(...) explicitly catch (AssumptionViolatedException ae) ... and immediately re-throw?

[Legacy Jira: Chris M. Hostetter on Oct 03 2019]

mikemccand commented 4 years ago

Attaching a patch where i started pursuing this idea – not just for AssumptionViolatedException but also for AssertionError, so that if someone has an assertEquals(...) that fails somewhere down the stack inside their expectThrows(Foo.class, ...) call, the test will fail with that message, not one that says "Unexpected exception type, expected Foo but got AssertionError"

The patch also handles the possibility that the caller may explicitly pass AssumptionViolatedException.class or AssertionError.class to expectThrows(...) (something our tests do a surprisingly non-zero number of times) and it still does what the caller would expect: returning the caught AssumptionViolatedException/AssertionError instead of re-throwing it

I haven't run the full test suite yet, but so far it seems to work well ... what do folks think?

/cc ~dawid.weiss, @rjernst, @munendrasn, @gerlowskija

[Legacy Jira: Chris M. Hostetter on Oct 04 2019]

mikemccand commented 4 years ago

@hossman Thanks for fixing this. I missed it in LUCENE-8938 All changes LGTM. If I may point to some typos (would be nice if corrected :) )

In javadoc for _expectThrows in LuceneTestCase, linke should have been link
In TestExpectThrows, propogated should have been propagated

[Legacy Jira: Munendra S N on Oct 04 2019]

mikemccand commented 4 years ago

✔ +1 overall

Vote	Subsystem	Runtime	Comment
Prechecks
+1	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
master Compile Tests
+1	compile	0m 22s	master passed
Patch Compile Tests
+1	compile	0m 30s	the patch passed
+1	javac	0m 30s	the patch passed
+1	Release audit (RAT)	0m 30s	the patch passed
+1	Check forbidden APIs	0m 30s	the patch passed
+1	Validate source patterns	0m 30s	the patch passed
Other Tests
+1	unit	4m 53s	test-framework in the patch passed.
	10m 28s

Subsystem	Report/Notes
JIRA Issue	LUCENE-8999
JIRA Patch URL	https://issues.apache.org/jira/secure/attachment/12982179/LUCENE-8999.patch
Optional Tests	compile javac unit ratsources checkforbiddenapis validatesourcepatterns
uname	Linux lucene2-us-west.apache.org 4.4.0-112-generic #135-Ubuntu SMP Fri Jan 19 11:48:36 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool	ant
Personality	/home/jenkins/jenkins-slave/workspace/PreCommit-LUCENE-Build/sourcedir/dev-tools/test-patch/lucene-solr-yetus-personality.sh
git revision	master / b51013a
ant	version: Apache Ant(TM) version 1.9.6 compiled on July 20 2018
Default Java	LTS
Test Results	https://builds.apache.org/job/PreCommit-LUCENE-Build/209/testReport/
modules	C: lucene/test-framework U: lucene/test-framework
Console output	https://builds.apache.org/job/PreCommit-LUCENE-Build/209/console
Powered by	Apache Yetus 0.7.0 http://yetus.apache.org

This message was automatically generated.

[Legacy Jira: Lucene/Solr QA on Oct 04 2019]

mikemccand commented 4 years ago

LGTM.

[Legacy Jira: Dawid Weiss (@dweiss) on Oct 04 2019]

mikemccand commented 4 years ago

LGTM too. Maybe consider giving back a similar change in junit5 (https://github.com/junit-team/junit5/blob/master/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java) which is what the idea of expectThrows was based on.

[Legacy Jira: Ryan Ernst (@rjernst) on Oct 05 2019]

mikemccand commented 4 years ago

Thanks folks, I'll fix the typos (it scares me that that precommit/documentation-lint didn't detect that '{@linke ' vs '{@link ' mistake) and commit shortly.

...Maybe consider giving back a similar change in junit5...

I'll open an issue suggesting the improvement for AssertThrows and post bidirectional links, but i don't think I'm emotionally invested enough to jump through the hoops of creating a patch/PR against that code base.

[Legacy Jira: Chris M. Hostetter on Oct 07 2019]

mikemccand commented 4 years ago

Commit 4d0afd4afffaeeb652da097ab2c2d44af8cd5083 in lucene-solr's branch refs/heads/master from Chris M. Hostetter https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=4d0afd4

LUCENE-8999: LuceneTestCase.expectThrows now propogates assert/assumption failures up to the test w/o wrapping in a new assertion failure unless the caller has explicitly expected them

[Legacy Jira: ASF subversion and git services on Oct 07 2019]

mikemccand commented 4 years ago

Commit a20ab051ad817500329dc7e2ad4ace48e161a165 in lucene-solr's branch refs/heads/branch_8x from Chris M. Hostetter https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=a20ab05

LUCENE-8999: LuceneTestCase.expectThrows now propogates assert/assumption failures up to the test w/o wrapping in a new assertion failure unless the caller has explicitly expected them

(cherry picked from commit 4d0afd4afffaeeb652da097ab2c2d44af8cd5083)

[Legacy Jira: ASF subversion and git services on Oct 07 2019]

mikemccand commented 4 years ago

https://github.com/junit-team/junit5/issues/2050

[Legacy Jira: Chris M. Hostetter on Oct 07 2019]

mikemccand commented 4 years ago

Commit 4d0afd4afffaeeb652da097ab2c2d44af8cd5083 in lucene-solr's branch refs/heads/jira/SOLR-13821 from Chris M. Hostetter https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=4d0afd4

LUCENE-8999: LuceneTestCase.expectThrows now propogates assert/assumption failures up to the test w/o wrapping in a new assertion failure unless the caller has explicitly expected them

[Legacy Jira: ASF subversion and git services on Oct 08 2019]

mikemccand commented 2 years ago

Closing after the 9.0.0 release

[Legacy Jira: Adrien Grand (@jpountz) on Dec 08 2021]

mikemccand / stargazers-migration-test

expectThrows doesn't play nicely with "assume" failures [LUCENE-8999] #996

Legacy Jira details