RFR: 8350964: Add an ArtifactResolver.fetch(clazz) method [v3]

[Weijun Wang]

On Wed, 12 Mar 2025 15:29:36 GMT, Matthew Donovan <mdonovan at 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