On Fri, 14 Mar 2025 09:17:06 GMT, KIRIYAMA Takuya <d...@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
