-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8360882: Tests throw SkippedException when they should fail #26868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back mdonovan! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
@@ -48,43 +49,39 @@ public class OpensslArtifactFetcher { | |||
and return that path, if download fails then return null. | |||
* | |||
* @return openssl binary path of the current version | |||
* @throws SkippedException if a valid version of OpenSSL cannot be found | |||
* @throws IOException if a valid version of OpenSSL cannot be found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still also throws SkippedException
:
jdk/test/lib/jdk/test/lib/security/OpensslArtifactFetcher.java
Lines 83 to 84 in ae26c5a
throw new SkippedException(String.format("No OpenSSL %s found for %s/%s", | |
OPENSSL_BUNDLE_VERSION, Platform.getOsName(), Platform.getOsArch())); |
* @throws IOException if a valid version of OpenSSL cannot be found | |
* @throws IOException if a valid version of OpenSSL cannot be found | |
* @throws SkippedException if OpenSSL is not available on the target platform |
… would rarely be checked otherwise.
@@ -48,43 +49,40 @@ public class OpensslArtifactFetcher { | |||
and return that path, if download fails then return null. | |||
* | |||
* @return openssl binary path of the current version | |||
* @throws SkippedException if a valid version of OpenSSL cannot be found | |||
* @throws SkippedException if OpenSSL is not available on the target platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since SkippedException
is now thrown in all cases, the JavaDoc @throws
clauses should be merged.
* @throws SkippedException if OpenSSL is not available on the target platform | |
* @throws SkippedException if a valid version of OpenSSL cannot be found | |
* or if OpenSSL is not available on the target platform |
This PR updates
ArtifactResolver.fetchOne()
to throw an IOException instead of SkippedException. This allows tests to determine of a missing "artifact" should be treated as a failed or skipped test.OpensslArtifactFetcher
is also updated to only throw SkippedException if OpenSSL is not available on the test platform.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26868/head:pull/26868
$ git checkout pull/26868
Update a local copy of the PR:
$ git checkout pull/26868
$ git pull https://git.openjdk.org/jdk.git pull/26868/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26868
View PR using the GUI difftool:
$ git pr show -t 26868
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26868.diff
Using Webrev
Link to Webrev Comment