On Wed, 12 Mar 2025 15:29:36 GMT, Matthew Donovan <mdono...@openjdk.org> wrote:
>> In this PR, I created a new method, `ArtifactResolver.fetchOne()`, to
>> consolidate duplicate code across tests.
>
> Matthew Donovan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changed ArtifactResolver.fetchOne() to throw a skipped exception
test/jdk/sun/security/pkcs11/PKCS11Test.java line 749:
> 747: private static Path fetchNssLib(Class<?> clazz, Path libraryName)
> throws IOException {
> 748: Path p = ArtifactResolver.fetchOne(clazz);
> 749: return findNSSLibrary(p, libraryName);
So this method should never return null. Can we change `fetchNssLib(String
osId, Path libraryName)` to also never return null (default throws
`SkippedException`)? Then inside `getNSSLibPath(String library)` there is no
need for the null check.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 85:
> 83: if (generatePKCS12) {
> 84: String opensslPath = OpensslArtifactFetcher.getOpensslPath();
> 85: if (opensslPath == null) {
Can we modify `OpensslArtifactFetcher.getOpensslPath()` so that the
`SkippedException` is thrown inside it when the version check fails at the last
line? You are already throwing this exception when `fetchOpenssl` is called
there.
This makes sure the return value is never null, and it's consistent to
`ArtifactResolver.fetchOne`. You can add a `@throws` javadoc there for clarity.
test/jdk/sun/security/provider/acvp/Launcher.java line 29:
> 27: import jtreg.SkippedException;
> 28:
> 29: import java.io.IOException;
Useless now. The `import jtreg.SkippedException` is also useless.
test/lib/jdk/test/lib/artifacts/ArtifactResolver.java line 28:
> 26: import jtreg.SkippedException;
> 27:
> 28: import java.io.IOException;
Useless now.
test/lib/jdk/test/lib/artifacts/ArtifactResolver.java line 78:
> 76: * <p>
> 77: * Artifacts are defined with the {@link
> jdk.test.lib.artifacts.Artifact}
> 78: * annotation. The file name should have the format
> ARTIFACT_NAME-VERSION.EXT
It could also be a directory, right? So maybe `path name` is more precise.
Also, you haven't defined what each piece in ARTIFACT_NAME-VERSION.EXT is. On
the other hand, the names in `Artifact` are `name`, `revision`, and `extension`.
test/lib/jdk/test/lib/artifacts/ArtifactResolver.java line 81:
> 79: * <p>
> 80: * If you have a local version of a dependency that you want to use,
> you can
> 81: * specify that by setting the System property:
No need to capitalize `system`.
test/lib/jdk/test/lib/artifacts/ArtifactResolver.java line 92:
> 90: * @return the local path to the artifact. If the artifact is a
> compressed
> 91: * file that gets unpacked, this path will point to the root
> 92: * directory of the uncompressed file.
Maybe `file(s)`?
test/lib/jdk/test/lib/security/OpensslArtifactFetcher.java line 26:
> 24: package jdk.test.lib.security;
> 25:
> 26: import java.io.IOException;
Useless now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991892312
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991851644
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991834832
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991857027
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991859352
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991860392
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991866853
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1991874705
