RFR: 8352016: Improve java/lang/RuntimeTests/RuntimeExitLogTest.java

[Mikhail Yankelevich]

On Fri, 14 Mar 2025 09:17:06 GMT, KIRIYAMA Takuya <duke at openjdk.org> wrote:

> The current test program for the logging feature added in JDK-8301627 does not fully check some important cases.
> 
> Issue Details:
> The test does not properly check cases where logging might not happen due to different logging levels. (e.g. ALL, TRACE, WARNING, etc.)
> The check for the logged stack trace is not enough, as it does not confirm enough details in the output.
> 
> Fix Details:
> Added more test cases to check behavior under different logging levels.
> Improved the stack trace check by verifying more details in the logged output.
> These changes make the test more complete and ensure that the logging feature works as expected.
> Also, any existing test cases prior to this pull request are retained.
> 
> The test was verified in the following OS environments, and it passed successfully in both environments.
> - Windows Server 2022 Standard 21H2
> - Red Hat Enterprise Linux release 9.2 (Plow)
> 
> Could you please review this fix?

Looks good, just a minor comment. Thank you!

test/jdk/java/lang/RuntimeTests/RuntimeExitLogTest.java line 52:

> 50:     private static final String TEST_JDK = System.getProperty("test.jdk");
> 51:     private static final String TEST_SRC = System.getProperty("test.src");
> 52:     private static final String NL = System.getProperty("line.separator");

Wouldn't it be easier to just use `System.lineSeparator();` here, what do you think? 

Also I'd personally rename it to `NEW_LINE` or `SEPARATOR`, as it took me a few read throughs to understand what does NL stand for, but thats minor.

test/jdk/java/lang/RuntimeTests/RuntimeExitLogTest.java line 53:

> 51:     private static final String TEST_SRC = System.getProperty("test.src");
> 52:     private static final String NL = System.getProperty("line.separator");
> 53:     private static Object HOLD_LOGGER;

Nitpick: This is not used, could you please remove if there will be another revision.

test/jdk/java/lang/RuntimeTests/RuntimeExitLogTest.java line 167:

> 165:                 List<String> lines = reader.lines().toList();
> 166:                 boolean match = (expectMessage.isEmpty())
> 167:                         ? lines.size() == 0

Nitpick: `? lines.isEmpty()` would be easier to read imo, but it's fine as it is.

test/jdk/java/lang/RuntimeTests/RuntimeExitLogTest.java line 174:

> 172:                     System.err.println(expectMessage.replaceAll("\\n", NL));
> 173:                     System.err.println("---- Actual output begin");
> 174:                     lines.forEach(l -> System.err.println(l));

Nitpick: `lines.forEach(System.err::println);` would be easier to read in my opinion, but it's fine as it is.

-------------

PR Review: https://git.openjdk.org/jdk/pull/24050#pullrequestreview-2694348466
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2000948118
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2000943776
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2000950379
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2000949810