On Wed, 12 Mar 2025 16:39:41 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> 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.
Yep, i removed the unnecessary null-checks
> 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`.
This sentence doesn't really belong here anyway so I removed it entirely
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1992239433
PR Review Comment: https://git.openjdk.org/jdk/pull/23989#discussion_r1992239895
