On Tue, 26 Nov 2024 19:04:47 GMT, Valerie Peng <valer...@openjdk.org> wrote:
> Could someone please help review this trivial fix?
>
> The constants for ML-KEM are accidentally removed during merge. This PR adds
> them back and adds a test for checking them against the expected ones.
A few small comments but this looks fine in order to get this fixed quickly.
However, I think it would be better if there were ML-KEM and ML-DSA specific
tests that used the public constants. I think we should followup on that with
Ben and Rajan and possibly open a separate issue to improve existing tests.
test/jdk/java/security/spec/TestNamedParameterSpec.java line 33:
> 31: import java.lang.reflect.Field;
> 32: import java.lang.reflect.Modifier;
> 33: import java.security.spec.*;
Nit: can you just import `NamedParameterSpec`?
test/jdk/java/security/spec/TestNamedParameterSpec.java line 60:
> 58: public static String[] getSortedConstNames(Class<?> clazz) {
> 59: TreeSet<String> names = new TreeSet<String>();
> 60: for(Field field : clazz.getDeclaredFields()){
Nit: add space after "for".
-------------
Marked as reviewed by mullan (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/22397#pullrequestreview-2462492663
PR Review Comment: https://git.openjdk.org/jdk/pull/22397#discussion_r1859113300
PR Review Comment: https://git.openjdk.org/jdk/pull/22397#discussion_r1859111279
