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

[Pavel Rappo]

On Wed, 5 Jun 2024 17:48:25 GMT, Naoto Sato <naoto at 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