On Fri, 8 Nov 2024 18:00:42 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 13 additional > commits since the last revision: > > - Merge branch 'master' into 8342442 > - more precise ML-DSA sigVer negative test > - more clear exception message > - check provider availability; SourceRandom in lib; use Utils.toByteArray > - rename property names, add an example > - acvp.test.alg system property > - remove plugin design > - rename > - grammar issues > - license > - ... and 3 more: https://git.openjdk.org/jdk/compare/d3963867...a7e22873 test/jdk/sun/security/provider/acvp/Launcher.java line 1: > 1: /* By default, does this test run and do nothing because there are no `internalProjection.json` files? Are those going to be added as test data files later? Should the test be reported as skipped if there is nothing to test? test/jdk/sun/security/provider/acvp/Launcher.java line 36: > 34: */ > 35: > 36: /// This test runs on `internalProjection.json`-style files generated Is there a reason you didn't put all these comments (or a copy of them) in the README file instead? It seems like a better place for it. The README doesn't help me understand how to run the tests. test/jdk/sun/security/provider/acvp/Launcher.java line 43: > 41: /// The test walks through the directory recursively and looks for > 42: /// file names equals to or ending with `internalProjection.json` and > 43: /// runs test on them. s/test/tests/ test/jdk/sun/security/provider/acvp/Launcher.java line 45: > 43: /// runs test on them. > 44: /// > 45: /// Set the `test.acvp.alg` test property to only test this algorithm. What is "this algorithm"? Should it maybe be "the specified algorithm"? test/jdk/sun/security/provider/acvp/Launcher.java line 51: > 49: /// > 50: /// By default, the test uses system-preferred implementations. > 51: /// If you want to test on a specific provider, set the s/on a/a/ ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1835067732 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1835055952 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1835056533 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1835057439 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1835058786
