RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]

[Naoto Sato]

On Thu, 6 Jun 2024 09:05:23 GMT, Pavel Rappo <prappo at 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