On Tue, 17 Jun 2025 13:08:24 GMT, Mikhail Yankelevich
<myankelev...@openjdk.org> wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> enhance test to be exhaustive
>
> src/java.base/share/classes/sun/security/util/KeyUtil.java line 150:
>
>> 148: && ak.getParams() instanceof NamedParameterSpec nps)
>> 149: ? nps.getName()
>> 150: : k.getAlgorithm();
>
> This case is not covered. Could you please add something like this test? it
> should cover the case when this is false `k instanceof AsymmetricKey ak &&
> ak.getParams() instanceof NamedParameterSpec nps`
>
>
> static void testDummyKey() throws Exception {
>
> Key key = new Key() {
> @Override
> public String getAlgorithm() {
> return "";
> }
>
> @Override
> public String getFormat() {
> return "";
> }
>
> @Override
> public byte[] getEncoded() {
> return new byte[0];
> }
> };
>
> Asserts.assertEQ(-1, KeyUtil.getNistCategory(key));
> }
> ```
> and `testDummyKey();` on line 52 of course.
Added a test case on `SecretKey`, which is not an `AsymmetricKey`.
> test/jdk/sun/security/provider/all/NistCategories.java line 46:
>
>> 44: for (var p : Security.getProviders()) {
>> 45: for (var s : p.getServices()) {
>> 46: switch (s.getType()) {
>
> Minor: why not just this? I don't mind, just asking
>
>
> for (var p : Security.getProviders()) {
> for (var s : p.getServices()) {
> if ("KeyPairGenerator".equals(s.getType())) {
> test(s);
> }
> }
> }
Oh, you're right it's not worth a switch. I might have just copied the same
structure from another test where there can be different actions on more than 1
type. I'll change it. Thanks.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25642#discussion_r2152342047
PR Review Comment: https://git.openjdk.org/jdk/pull/25642#discussion_r2152335707
