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/71ce3c80...a7e22873 test/jdk/sun/security/provider/acvp/Launcher.java line 42: > 40: /// directory specified by the `test.acvp.data` test property. > 41: /// The test walks through the directory recursively and looks for > 42: /// file names equals to or ending with `internalProjection.json` and I think it should be "equal to". test/jdk/sun/security/provider/acvp/Launcher.java line 77: > 75: var p = Security.getProvider(provProp); > 76: if (p == null) { > 77: System.err.println(provProp + " is not a registered > provider name"); Seems odd to print the error message and throw an exception with the same message. test/jdk/sun/security/provider/acvp/ML_DSA_Test.java line 85: > 83: public byte[] getEncoded() { return > toByteArray(c.get("sk").asString()); } > 84: }; > 85: var sr = new FixedSecureRandom(det ? new byte[32] : > toByteArray(c.get("rnd").asString())); This line and others are greater than 80 characters. test/jdk/sun/security/provider/acvp/ML_DSA_Test.java line 89: > 87: s.update(toByteArray(c.get("message").asString())); > 88: var sig = s.sign(); > 89: Asserts.assertEqualsByteArray(sig, > toByteArray(c.get("signature").asString())); If more than one test fails, it might be useful to print out all the failures and then, at the end, throw an assert saying one or more tests failed. test/jdk/sun/security/provider/acvp/ML_KEM_Test.java line 60: > 58: System.out.print(c.get("tcId").asString() + " "); > 59: g.initialize(np, new FixedSecureRandom( > 60: toByteArray(c.get("d").asString()), > toByteArray(c.get("z").asString()))); This line and others below are way over 80 characters. test/lib/jdk/test/lib/security/FixedSecureRandom.java line 28: > 26: import java.security.SecureRandom; > 27: > 28: /// A custom implementation of `SecureRandom` that outputs a Anything special about `///`? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1834912418 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1834922801 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1834941865 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1834967912 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1834975363 PR Review Comment: https://git.openjdk.org/jdk/pull/21548#discussion_r1835028741
