On Mon, 18 May 2026 06:47:43 GMT, SendaoYan <[email protected]> wrote:
>> Rajan Halade has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typos
>
> test/jdk/sun/security/ssl/X509KeyManager/PreferredKey.java line 35:
>
>> 33: * @summary X509KeyManager implementation for NewSunX509 doesn't return
>> most
>> 34: * preferable key
>> 35: * @run main/othervm PreferredKey
>
> Should we keep the new java process flag 'main/othervm'
There is no need to as no system properties are modified.
> test/jdk/sun/security/ssl/X509KeyManager/PreferredKey.java line 103:
>
>> 101: .setPublicKey(caKeys.getPublic())
>> 102: .setNotBefore(Date.from(Instant.now().minus(1,
>> ChronoUnit.HOURS)))
>> 103: .setNotAfter(Date.from(Instant.now().plus(1,
>> ChronoUnit.HOURS)))
>
> 1-hour lifetime is enough for normal jtreg runs but maybe fragile.
Why does it need to be valid for time when test is expected to finish within a
second?
> test/jdk/sun/security/ssl/X509KeyManager/SelectOneKeyOutOfMany.java line 85:
>
>> 83:
>> 84: // String chooseClientAlias(String[] keyType, Principal[]
>> issuers, Socket socket)
>> 85: Asserts.assertNull(km.chooseClientAlias(new String[]{NOTHING},
>> null, null),
>
> The message says getServerAliases but the method under test is
> chooseClientAlias
thanks, I have fixed this.
> test/jdk/sun/security/ssl/X509KeyManager/SelectOneKeyOutOfMany.java line 107:
>
>> 105: // String chooseServerAlias(String keyType, Principal[]
>> issuers, Socket socket)
>> 106: Asserts.assertNull(km.chooseServerAlias(NOTHING, null, null),
>> 107: "getServerAliases shouldn't return alias for
>> unsupported type");
>
> The message says getServerAliases but the method under test is
> chooseServerAlias.
thanks, I have fixed this.
> test/jdk/sun/security/ssl/X509KeyManager/SelectOneKeyOutOfMany.java line 143:
>
>> 141: }
>> 142:
>> 143: private static X509Certificate createSelfSignedCert(KeyPair caKeys,
>> String keyAlg)
>
> Duplicated createSelfSignedCert in both files. Should we consider a
> package-private helper in the same directory or a shared test utility in a
> follow-up to reduce drift
Yes, There are other tests that use `CertificateBuilder` and has duplicate
code. We should plan to move those to library. I will follow up on that as a
separate fix.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31186#discussion_r3260675756
PR Review Comment: https://git.openjdk.org/jdk/pull/31186#discussion_r3260677996
PR Review Comment: https://git.openjdk.org/jdk/pull/31186#discussion_r3260677395
PR Review Comment: https://git.openjdk.org/jdk/pull/31186#discussion_r3260676902
PR Review Comment: https://git.openjdk.org/jdk/pull/31186#discussion_r3260676189
