On Thu, 6 Mar 2025 20:10:58 GMT, Sean Coffey <coff...@openjdk.org> wrote:
>> Breaking the parent JDK-8044609 JBS issue into sub tasks.
>>
>> This patch addresses the main issue which is that `javax.net.debug=ssl `
>> option is completely broken since TLSv1.3 support was introduced. This
>> patch should be easier for backporting also.
>>
>> Wider corrections can be followed up via parent bug.
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Incorporate latest review feedback
Depending on the followup discussion of what `ssl` alone means.
However, several suggestions below, one might help with test coverage. If you
take some/all, then I'll review again. Should be quick.
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 1:
> 1: /*
A suggestion that could ease/increase test coverage:
HashMap<String, String> masterMap = new HashMap<>();
// add one <String, String> for each output category.
masterMap.put("handshake", "Produced ClientHello handshake message");
// ...
// for each testcase {
// missing = clone masterMap; // start with the full map
// required = new HashMap(); // create an empty map
// for each key in the test case {
// missing.remove(value);
// required.add(value);
// }
// runTest();
// check each value String in required is present
// check each value String in missing is not
// }
You can then easily add test cases with:
test("ssl","handshake"); // use "..." params if you don't want to do
String parsing, and have multiple params
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 27:
> 25: * @test
> 26: * @bug 8350582
> 27: * @library /test/lib /javax/net/ssl/templates ../../
Is `../..` actually needed?
It ran fine in another test directory several levels below this one.
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 28:
> 26: * @bug 8350582
> 27: * @library /test/lib /javax/net/ssl/templates ../../
> 28: * @summary Verify debug output for different javax.net.debug scenarios
Isn't the summary supposed to match the bugid's description?
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 59:
> 57:
> 58: private static Stream<Arguments> patternMatches() {
> 59: // "Plaintext before ENCRYPTION" comes from
> "ssl:record:plaintext" option
Minor nits. >= 80 chars in some lines.
You could leave as is, or shorten the searched-for message. I'm ok if you
really want to leave, as I don't expect lots of changes in this file.
(horizontal scrolling during reviews is my pet peeve)
Or you could do:
`Produced ClientHello handshake message`
->
`Produced ClientHello handshake`
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 168:
> 166: }
> 167:
> 168: public static void main(String[] args) throws Exception {
Is this here because for running outside of jtreg, say via the command line?
-------------
Marked as reviewed by wetmore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/23781#pullrequestreview-2755088852
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036411141
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036335133
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036416941
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036338994
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036419493
