On Mon, 14 Jul 2025 17:57:22 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test/jdk/java/io/Console/LocaleTest.java
>>
>> Co-authored-by: Andrey Turbanov <turban...@gmail.com>
>
> test/jdk/java/io/Console/DefaultCharsetTest.java line 52:
>
>> 50: void testDefaultCharset(String stdoutEncoding) throws Exception {
>> 51: // check "expect" command availability
>> 52: var expect = Paths.get("/usr/bin/expect");
>
> We only need to check "expect" availability once, so we should move this
> check to a `@BeforeAll` static method. It's also more clear that this check
> is a precondition, and not part of the actual test. Applies to the other
> locations, but primarily the other parameterized tests.
Good point. Modified to use `@BeforeAll`. For `ModuleSelectionTest`, one of the
test is not using `expect`, so I left it as it is. In addition to that, I
removed the `@requires` condition to allow that test to run on windows.
> test/jdk/java/io/Console/ModuleSelectionTest.java line 75:
>
>> 73: @ParameterizedTest
>> 74: @MethodSource("options")
>> 75: void testWithExpect(String opts, String expected) throws Exception {
>
> I think it would be more clear if these tests were renamed to
> `expectConsoleTest` and `noConsoleTest` or something along those lines.
Yeah, using `expect` command with junit's expect is confusing 🙂. Just renamed
those methods using TTY/NonTTY, as"expect/noConsoleTest" reads somewhat odd as
it includes the expected results in the test name.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26273#discussion_r2205749625
PR Review Comment: https://git.openjdk.org/jdk/pull/26273#discussion_r2205753058
