On Tue, 25 Feb 2025 16:20:59 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.
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValues.java line 47:
> 45: import jdk.test.lib.process.OutputAnalyzer;
> 46:
> 47: public class DebugPropertyValues extends SSLSocketTemplate {
Do you think test name should end with `Test` if it runs with Junit? AFAIK the
convention for the naming looks like `[A-Z][A-Za-z\d]*Test(s|Case)?`
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValues.java line 49:
> 47: public class DebugPropertyValues extends SSLSocketTemplate {
> 48:
> 49: static Path LOG_FILE;
Should this file be in the format of a constant? It's changed by the code so it
seems to me it can be either changed to `logFile` or line 53 could be moved
straight here (something like `private static final Path LOG_FILE =
Path.of(...`).
What do you think?
test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValues.java line 53:
> 51: @BeforeAll
> 52: static void setup() throws Exception {
> 53: LOG_FILE = Path.of(System.getProperty("test.classes"),
> "logging.conf");
I think this should go the the
[scratch](https://openjdk.org/jtreg/faq.html#scratch-directory) directory, as
this file is temporary.
This would look like `Path.of("logging.conf");`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r1973227742
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r1973246148
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r1973236305
