On Fri, 1 Nov 2024 18:32:53 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Here we have a launcher and several algorithm-specific tests. Users can
>> populate "internalProjection.json" files generated by NIST's ACVP Server
>> into the `data` directory and test them with the launcher.
>>
>> Currently, only SHA2, SHA3, ML-KEM, and ML-DSA are supported.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> rename property names, add an example
The duplication in utilities (RandomSource and xeh(String) should be avoided.
Perhaps in the test library or a common utility class.
test/jdk/sun/security/provider/acvp/Launcher.java line 34:
> 32: * @bug 8342442
> 33: * @library /test/lib
> 34: */
This seems more like an '@driver' type of test than the implied `@run` test.
test/jdk/sun/security/provider/acvp/Launcher.java line 45:
> 43: var provProp = System.getProperty("test.acvp.provider");
> 44: PROVIDER = provProp != null
> 45: ? Security.getProvider(provProp)
How are errors in the provider prop reported? In a static block, will an
uncaught exception provide enough/correct information to correct the supplied
properties.
test/jdk/sun/security/provider/acvp/Launcher.java line 78:
> 76: // -Dtest.acvp.alg=ML-KEM \
> 77: // -Dtest.acvp.data=/path/to/json-files/ \
> 78: // -jdk:/path/to/jdk Launcher.java
This seems more like either the class javadoc or the javadoc for main(). (Not
an inline comment).
test/jdk/sun/security/provider/acvp/Launcher.java line 120:
> 118: case "SHA2-256", "SHA2-224", "SHA3-256", "SHA3-224"
> 119: -> SHA_Test.run(kat, PROVIDER);
> 120: default -> System.out.println("Skipped unsupported
> algorithm: " + alg);
This seems more like a test configuration error and should not silently be
ignored (you have to read the output to see the error/warning).
Perhaps it could be a "Skipped" kind of test, so it gets reported.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21548#pullrequestreview-2410805715
PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1826250496
PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1826249022
PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1826248157
PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1826251779
