8332161: Test restoring echo in the Console implementation (java.base) #19315

naotoj · 2024-05-20T18:08:31Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8332161: Test restoring echo in the Console implementation (java.base) (Bug - P4)(⚠️ The fixVersion in this issue is [23] but the fixVersion in .jcheck/conf is 24, a new backport will be created when this pr is integrated.)

Reviewers

Joe Wang (@JoeWang-Java - Reviewer) ⚠️ Review applies to c583c633
Pavel Rappo (@pavelrappo - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315
$ git checkout pull/19315

Update a local copy of the PR:
$ git checkout pull/19315
$ git pull https://git.openjdk.org/jdk.git pull/19315/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19315

View PR using the GUI difftool:
$ git pr show -t 19315

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19315.diff

Webrev

Link to Webrev Comment

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

…oreEcho-Test

bridgekeeper · 2024-05-20T18:09:09Z

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into pr/19184 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-05-20T18:09:33Z

@naotoj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8332161: Test restoring echo in the Console implementation (java.base)

Reviewed-by: joehw, prappo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 83 new commits pushed to the master branch:

40b2fbd: 8331733: [PPC64] saving and restoring CR is not needed at most places
6968770: 8331935: Add support for primitive array C1 clone intrinsic in PPC
a2030ff: 8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs
bf7f1c4: 8333211: NMT Reports: replace manual indentation handling with auto indent
8ffc35d: 8333724: Problem list security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java#teliasonerarootcav1
f7862bd: 8331311: C2: Big Endian Port of 8318446: optimize stores into primitive arrays by combining values into larger store
b4beda2: 8332537: C2: High memory usage reported for compiler/loopopts/superword/TestAlignVectorFuzzer.java
e5383d7: 8333713: C2 SuperWord: cleanup in vectornode.cpp/hpp
944aeb8: 8325155: C2 SuperWord: remove alignment boundaries
d8af589: 8026127: Deflater/Inflater documentation incomplete/misleading
... and 73 more: https://git.openjdk.org/jdk/compare/9686e804a2b058955ff88149c54a0a7896c0a2eb...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-05-20T18:10:04Z

@naotoj The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-05-20T18:13:16Z

Webrevs

…oreEcho-Test

openjdk-notifier · 2024-05-24T17:54:30Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8332161-restoreEcho-Test
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-05-24T17:55:38Z

@naotoj this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8332161-restoreEcho-Test
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-05-24T17:55:42Z

⚠️ @naotoj This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

…oreEcho-Test

pavelrappo · 2024-05-29T14:10:40Z

I might be mistaken, but this looks like a very well written unit test, which tests that an implementation flag retains its initial value. This does not look like an end-to-end test, which we usually write.

I wonder if we can use expect to run two end-to-end tests as follows. The first test would set up a console with echo on and then verify that after the spawned command completes the echo is still on. The second test would do the same for echo off.

The difference between the above approach and the test that this PR has proposed is that with the above approach we won't rely on an implementation detail, whose relevance to an objectively observed state is non-obvious or unknown. Instead, we'll use an OS utility, which makes for a relevant, straightforward, and maintainable test.

pavelrappo

This feels like a better, end-to-end test. Thank you for your perseverance. Below are some comments.

pavelrappo · 2024-06-06T09:05:23Z

test/jdk/java/io/Console/RestoreEchoTest.java

+        var jdkDir = System.getProperty("test.jdk");
+        OutputAnalyzer output = ProcessTools.executeProcess(
+                "expect",
+                "-n",


What does -n do?

It is for not reading the user's expect settings (~/.expect.rc if any)

pavelrappo · 2024-06-06T11:22:07Z

test/jdk/java/io/Console/RestoreEchoTest.java

+                testSrc + "/restoreEcho.exp",
+                initialEcho,
+                jdkDir + "/bin/java",
+                "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED",


We don't seem to need --add-opens.

pavelrappo · 2024-06-06T11:32:39Z

test/jdk/java/io/Console/RestoreEchoTest.java

+                jdkDir + "/bin/java",
+                "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED",
+                "-Djdk.console=java.base",
+                "-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.

Turned out that removing the classpath ends up not finding the test class:

Error: Could not find or load main class RestoreEchoTest Caused by: java.lang.ClassNotFoundException: RestoreEchoTest ];

Turned out that removing the classpath ends up not finding the test class:

Error: Could not find or load main class RestoreEchoTest Caused by: java.lang.ClassNotFoundException: RestoreEchoTest ];

Hm... this is surprising. On my machine, testSrc expands to the JTwork/classes/0/java/io/Console/RestoreEchoTest.d directory, which is included in CLASSPATH and which contains RestoreEchoTest.class.

pavelrappo · 2024-06-06T11:40:20Z

test/jdk/java/io/Console/RestoreEchoTest.java

+        if (eval != 0) {
+            throw new RuntimeException("Test failed. Exit value from 'expect' command: " + eval);
+        }


It could've been assertEquals(0, output.getExitValue()).

pavelrappo · 2024-06-06T11:41:40Z

test/jdk/java/io/Console/RestoreEchoTest.java

+
+        // testing readLine()
+        String input = con.readLine("prompt: ");
+        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.

pavelrappo · 2024-06-06T11:41:53Z

test/jdk/java/io/Console/RestoreEchoTest.java

+
+        // testing readPassword()
+        input = String.valueOf(con.readPassword("password prompt: "));
+        con.printf("password is %s\n", input);


Ditto on %n.

pavelrappo · 2024-06-06T12:30:38Z

test/jdk/java/io/Console/restoreEcho.exp

+# readPassword() - input is not displayed
+test "$rpprompt" "$rpinput" "-echo" "$rpexpected"
+# See if the initialEcho is restored with `stty -a`
+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.

It's clever. However, we now depend on (-)echo being in the middle of the line, no? If stty -a format (IEEE Std 1003.2) allows (-)echo to appear in an arbitrary position of a line, our check won't work.

If (-)echo appears in a leading position, it might be preceded by a TAB (similarly to -echoprt below). If (-)echo appears in a trailing position, it is followed by newline (similarly to echoctl below).

lflags: icanon isig iexten echo echoe -echok echoke -echonl echoctl$ ^I-echoprt -altwerase -noflsh -tostop -flusho pendin -nokerninfo$ ^I-extproc$

But I guess, it's good enough and certainly much better than the initially proposed unit test.

pavelrappo

Thanks for having done it, Naoto.

naotoj · 2024-06-07T16:21:19Z

/integrate
Thanks for the reviews!

openjdk · 2024-06-07T16:22:04Z

Going to push as commit 25ad862.
Since your change was applied there have been 87 commits pushed to the master branch:

ee82346: 8325984: 4 jcstress tests are failing in Tier6 4 times each
d744059: 8333774: Avoid eagerly loading various EmptySpliterator classes
d130d2f: 8333477: Delete extra empty spaces in Makefiles
486dee2: 8333653: Remove MallocHeader::get_stack
40b2fbd: 8331733: [PPC64] saving and restoring CR is not needed at most places
6968770: 8331935: Add support for primitive array C1 clone intrinsic in PPC
a2030ff: 8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs
bf7f1c4: 8333211: NMT Reports: replace manual indentation handling with auto indent
8ffc35d: 8333724: Problem list security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java#teliasonerarootcav1
f7862bd: 8331311: C2: Big Endian Port of 8318446: optimize stores into primitive arrays by combining values into larger store
... and 77 more: https://git.openjdk.org/jdk/compare/9686e804a2b058955ff88149c54a0a7896c0a2eb...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-07T16:22:15Z

@naotoj Pushed as commit 25ad862.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

naotoj and others added 11 commits May 10, 2024 14:49

initial commit

43f555e

Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java

9daf699

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

Replaced another unused exception with _

3e7d2e0

Addresses a review comment

fb5beb1

get/setEcho()

898c609

Merge branch 'master' into JDK-8332084-restoreEcho-readLock

e178f0d

copyright year

82d30b3

Merge branch 'master' into JDK-8332084-restoreEcho-readLock

56d95e3

initial commit

5815180

Merge branch 'master' into JDK-8332084-restoreEcho-readLock

e58fbdc

Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-rest…

f272f30

…oreEcho-Test

openjdk bot added the rfr Pull request is ready for review label May 20, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label May 20, 2024

naotoj changed the title ~~8332161: Need a test for restoring echo in the Console implementation (java.base)~~ 8332161: Test restoring echo in the Console implementation (java.base) May 20, 2024

naotoj added 8 commits May 21, 2024 15:33

Used a dedicate lock for restoreEcho

f69f747

Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-rest…

c94b290

…oreEcho-Test

get/setEcho() -> echo()

5392455

leftover typo

ad4a4e4

Merge branch 'master' into JDK-8332084-restoreEcho-readLock

69ec27d

Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-rest…

c8e15b8

…oreEcho-Test

fixing typo

1cd88c6

Added comment for restoreEchoLock

d4b6e69

openjdk-notifier bot changed the base branch from pr/19184 to master May 24, 2024 17:54

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label May 24, 2024

Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-rest…

7311487

…oreEcho-Test

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label May 24, 2024

naotoj added 3 commits May 24, 2024 11:48

Merge branch 'master' into JDK-8332161-restoreEcho-Test

36649fb

Merge branch 'master' into JDK-8332161-restoreEcho-Test

5bd519e

incorporated the same fix from 8332922

2477adf

JoeWang-Java approved these changes May 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 29, 2024

naotoj added 4 commits June 4, 2024 10:40

Check restored echo with stty -a

c1ea5d5

Merge branch 'master' into JDK-8332161-restoreEcho-Test

279b80c

Corrected comments

1c0ab8e

Removed unnecessary add-opens

c583c63

JoeWang-Java approved these changes Jun 6, 2024

View reviewed changes

pavelrappo reviewed Jun 6, 2024

View reviewed changes

naotoj added 3 commits June 6, 2024 10:04

Reflects review comments

0e72fe4

Restored classpath

c647627

Loosening the timeout value

df5f8f9

pavelrappo approved these changes Jun 7, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 7, 2024

openjdk bot closed this Jun 7, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332161: Test restoring echo in the Console implementation (java.base) #19315

8332161: Test restoring echo in the Console implementation (java.base) #19315

naotoj commented May 20, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 20, 2024

openjdk bot commented May 20, 2024 •

edited

Loading

openjdk bot commented May 20, 2024

mlbridge bot commented May 20, 2024 •

edited

Loading

openjdk-notifier bot commented May 24, 2024

openjdk bot commented May 24, 2024

openjdk bot commented May 24, 2024

pavelrappo commented May 29, 2024

pavelrappo left a comment

pavelrappo Jun 6, 2024

naotoj Jun 6, 2024

pavelrappo Jun 6, 2024

pavelrappo Jun 6, 2024

naotoj Jun 6, 2024

naotoj Jun 6, 2024

pavelrappo Jun 6, 2024

pavelrappo Jun 6, 2024

pavelrappo Jun 6, 2024

pavelrappo Jun 6, 2024

pavelrappo Jun 6, 2024

naotoj Jun 6, 2024

pavelrappo Jun 6, 2024

pavelrappo left a comment

naotoj commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

8332161: Test restoring echo in the Console implementation (java.base) #19315

8332161: Test restoring echo in the Console implementation (java.base) #19315

Conversation

naotoj commented May 20, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 20, 2024

openjdk bot commented May 20, 2024 • edited Loading

openjdk bot commented May 20, 2024

mlbridge bot commented May 20, 2024 • edited Loading

Webrevs

openjdk-notifier bot commented May 24, 2024

openjdk bot commented May 24, 2024

openjdk bot commented May 24, 2024

pavelrappo commented May 29, 2024

pavelrappo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelrappo left a comment

Choose a reason for hiding this comment

naotoj commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

naotoj commented May 20, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 20, 2024 •

edited

Loading

mlbridge bot commented May 20, 2024 •

edited

Loading