On Thu, 23 Oct 2025 20:06:30 GMT, Bradford Wetmore <[email protected]> wrote:
>> Mikhail Yankelevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> line length changed
>
> test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 103:
>
>> 101: final X509KeyManager km = generateNullKm();
>> 102:
>> 103: km.getServerAliases(null, null);
>
> I know that this section is not part of this change, but shouldn't we be
> testing that all these are indeed returning null?
>
> It's done in spots later on, but I feel it should be done here also to test
> all of the APIs.
I don't think this test focuses on this, but in any case it's easy to add a
null verification, so I think it would be better to have it. Done in the next
commit
> test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 203:
>
>> 201: "Should return null if the alias is invalid
>> <%s>",
>> 202: alias));
>> 203: }
>
> Based on the description of the bug, I was expecting to see checking of the
> debug output (`Implement extra logging...`), but appears that this is not the
> primary focus. Maybe change the ordering of the bug description. (`...and
> also implement extra logging`)
I have removed log verification from the tests. I agree, changing the title
might be a good idea
> test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java
> line 70:
>
>> 68:
>> 69: exceptionThrowingKMF
>> 70: .init((KeyStore) exceptionThrowingKS, null);
>
> This cast seems unnecessary.
>
> (Test runs without it.)
Yes, I've removed it in the next commit, thanks.
> test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java
> line 83:
>
>> 81: @Test
>> 82: public void ksExceptionTest() {
>> 83: // recording logs to the output stream
>
> I don't understand this comment. There doesn't seem to be any recording to
> the debug log stream in this test.
That's a typo, I have modified a test which used to verify logs. Removed in the
next commit
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459620637
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459632554
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459627208
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459636148
