On Thu, 25 Jun 2026 19:31:04 GMT, Sean Mullan <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update java.security and Javadoc for getInstance()
>
> test/jdk/java/security/KeyStore/TestLegacyAlgorithms.java line 1:
>
>> 1: /*
>
> Suggest adding an additional test where you also disable the algorithm and
> check that a warning is not emitted
Added this test.
> test/jdk/java/security/KeyStore/TestLegacyAlgorithms.java line 56:
>
>> 54: List.of("JKS", "jkS");
>> 55:
>> 56: private static String saveWarn(ThrowingRunnable action) throws
>> Exception {
>
> Did you consider checking that each specific `getInstance` case produces the
> expected warning? I think it is harder to check all the warnings at the end
> and be sure all of them were produced as expected for all `getInstance` calls.
>
> Maybe if you flushed the stream, then checked the output for each test case,
> and then reset the `ByteArrayOutputStream` before the next test, and repeat
> for each test.
Fixed.
> test/jdk/java/security/KeyStore/TestLegacyAlgorithms.java line 77:
>
>> 75:
>> 76: KeyStore k;
>> 77: if (p == null) {
>
> This case is never tested AFAICT.
Fixed.
> test/jdk/java/security/KeyStore/TestLegacyAlgorithms.java line 135:
>
>> 133: "Expected legacy warning for KeyStore but not
>> found");
>> 134: for (String a : ALG_LIST) {
>> 135: Asserts.assertTrue(warnS.contains("WARNING: " + a +
>> warn2),
>
> This only checks if at least one of the `getInstance` calls emitted a
> warning. Can you check for all of them?
Done.
> test/jdk/java/security/KeyStore/TestLegacyAlgorithms.java line 142:
>
>> 140: "Expected warning not preserve caller: "
>> 141: + warnS);
>> 142: } else {
>
> It would also be useful to call the methods twice, and make sure you only get
> one warning per caller.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31472#discussion_r3510532302
PR Review Comment: https://git.openjdk.org/jdk/pull/31472#discussion_r3510533830
PR Review Comment: https://git.openjdk.org/jdk/pull/31472#discussion_r3510528096
PR Review Comment: https://git.openjdk.org/jdk/pull/31472#discussion_r3510532031
PR Review Comment: https://git.openjdk.org/jdk/pull/31472#discussion_r3510533003
