On Wed, 5 Jun 2024 17:48:25 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> This test intends to verify the behavior of JdkConsole for the java.base >> module, wrt restoring the echo. The test assumes the internal methods that >> sets/gets the echo status of the platform. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unnecessary add-opens This feels like a better, end-to-end test. Thank you for your perseverance. Below are some comments. test/jdk/java/io/Console/RestoreEchoTest.java line 66: > 64: OutputAnalyzer output = ProcessTools.executeProcess( > 65: "expect", > 66: "-n", What does `-n` do? test/jdk/java/io/Console/RestoreEchoTest.java line 70: > 68: initialEcho, > 69: jdkDir + "/bin/java", > 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED", We don't seem to need `--add-opens`. test/jdk/java/io/Console/RestoreEchoTest.java line 72: > 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED", > 71: "-Djdk.console=java.base", > 72: "-classpath", testClasses, Consider this. If we remove `-classpath` (and `var testClasses`), not only will nothing break, but we'll be also able to use JUnit assertions and assumptions in `main` instead of manual check-then-throw. This will work because the expect-process will inherit the environment, which captures `CLASSPATH` ( see https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ProcessBuilder.html#start() ). Again, the above is just something to consider. For all I know, you might've considered it already and rejected. test/jdk/java/io/Console/RestoreEchoTest.java line 78: > 76: if (eval != 0) { > 77: throw new RuntimeException("Test failed. Exit value from > 'expect' command: " + eval); > 78: } It could've been `assertEquals(0, output.getExitValue())`. test/jdk/java/io/Console/RestoreEchoTest.java line 93: > 91: // testing readLine() > 92: String input = con.readLine("prompt: "); > 93: con.printf("input is %s\n", input); I know that this test is only run on Linux and macOS, and yet `%n` would be better than `\n`. It's one of those cases where it's simpler to use a general solution than to use a less general one and explain why it still does the job. test/jdk/java/io/Console/RestoreEchoTest.java line 97: > 95: // testing readPassword() > 96: input = String.valueOf(con.readPassword("password prompt: ")); > 97: con.printf("password is %s\n", input); Ditto on `%n`. test/jdk/java/io/Console/restoreEcho.exp line 57: > 55: test "$rpprompt" "$rpinput" "-echo" "$rpexpected" > 56: # See if the initialEcho is restored with `stty -a` > 57: expect " $initialEcho " If you leave out those whitespace characters inside the quotes and `$initialEcho` expands to `-echo`, it will be treated as an option to `expect`, right? If so, consider this instead: expect -- $initialEcho But more importantly: could a test match `echo` in `-echo` and therefore falsely pass? ------------- PR Review: https://git.openjdk.org/jdk/pull/19315#pullrequestreview-2101329552 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629085477 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629331808 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629348345 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629359368 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629361442 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629361824 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629445000
