On Thu, 6 Jun 2024 09:05:23 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removed unnecessary add-opens > > test/jdk/java/io/Console/RestoreEchoTest.java line 66: > >> 64: OutputAnalyzer output = ProcessTools.executeProcess( >> 65: "expect", >> 66: "-n", > > What does `-n` do? It is for not reading the user's expect settings (`~/.expect.rc` if any) > 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. I haven't considered that. Removed. > 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? The spaces before/after `$initialEcho` are exactly to distinguish "echo" from "-echo", otherwise the test falsely succeeds as you pointed out. Although the test works as expected as it is, adding `--` would be safer. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915235 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915623 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915890
