On Tue, 18 Mar 2025 12:34:37 GMT, Mikhail Yankelevich
<[email protected]> wrote:
>> KIRIYAMA Takuya has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8352016: Improve java/lang/RuntimeTests/RuntimeExitLogTest.java
>
> 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.
I changed it as you said for readability.
> 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.
The "HOLD_LOGGER" variable is not directly used but is necessary for test
stability. It ensures that the ThrowingHandler remains attached to the
"java.lang.Runtime" logger and is not garbage collected.
> 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.
I see. I changed it as you said for readability.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2007132409
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2007121689
PR Review Comment: https://git.openjdk.org/jdk/pull/24050#discussion_r2007134108
