Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar [v2]
On Thu, 24 Mar 2022 20:59:20 GMT, Brent Christian wrote: >> Please review this change to the java/util/prefs/AddNodeChangeListener.jar >> test. >> >> Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh >> Preferences on each test run, MacOS does not seem to honor this, and still >> stores prefs in Library/. >> >> This test has long suffered intermittent failures. 8255031 added some >> debugging code; as of that change, the test fails fast as soon as an >> expected change event is not received. In particular, if the expected event >> is not received for adding the node, the test exits without removing the >> node. In this way, on Mac, the node can get "stuck" in the preferences of >> the machine. Future runs on the machine will already have the node, no node >> added change event will be generated (because the node was already there), >> the test will continue to fail without removing the node, etc. This matches >> observations on some CI machines. >> >> This change ensures that: >> * the test node is not present before the test >> * the test runs to completion, including removing the test node >> * the test node is not left behind after the test >> >> Thanks. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Add test failure message to 'couldn't remove' RuntimeException Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar [v2]
On Wed, 23 Mar 2022 17:29:04 GMT, Naoto Sato wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add test failure message to 'couldn't remove' RuntimeException
>
> test/jdk/java/util/prefs/AddNodeChangeListener.java line 63:
>
>> 61: } finally {
>> 62: // Make sure test node is not present after the test
>> 63: clearPrefs();
>
> If this call throws a `RuntimeException("Unable to clear...")`, it will
> override the possible `RuntimeException("Failed")` in the `try` block, but I
> think it is OK.
That's true - I hadn't considered that. I think it's OK, too. Even so, I've
added a message to the exception thrown from clearPrefs() to indicate if the
test failed.
-
PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar [v2]
> Please review this change to the java/util/prefs/AddNodeChangeListener.jar > test. > > Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh > Preferences on each test run, MacOS does not seem to honor this, and still > stores prefs in Library/. > > This test has long suffered intermittent failures. 8255031 added some > debugging code; as of that change, the test fails fast as soon as an expected > change event is not received. In particular, if the expected event is not > received for adding the node, the test exits without removing the node. In > this way, on Mac, the node can get "stuck" in the preferences of the machine. > Future runs on the machine will already have the node, no node added change > event will be generated (because the node was already there), the test will > continue to fail without removing the node, etc. This matches observations on > some CI machines. > > This change ensures that: > * the test node is not present before the test > * the test runs to completion, including removing the test node > * the test node is not left behind after the test > > Thanks. Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Add test failure message to 'couldn't remove' RuntimeException - Changes: - all: https://git.openjdk.java.net/jdk/pull/7908/files - new: https://git.openjdk.java.net/jdk/pull/7908/files/71ab1f17..139b6a94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7908=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7908=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7908.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7908/head:pull/7908 PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
On Tue, 22 Mar 2022 17:59:15 GMT, Brent Christian wrote:
> Please review this change to the java/util/prefs/AddNodeChangeListener.jar
> test.
>
> Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh
> Preferences on each test run, MacOS does not seem to honor this, and still
> stores prefs in Library/.
>
> This test has long suffered intermittent failures. 8255031 added some
> debugging code; as of that change, the test fails fast as soon as an expected
> change event is not received. In particular, if the expected event is not
> received for adding the node, the test exits without removing the node. In
> this way, on Mac, the node can get "stuck" in the preferences of the machine.
> Future runs on the machine will already have the node, no node added change
> event will be generated (because the node was already there), the test will
> continue to fail without removing the node, etc. This matches observations on
> some CI machines.
>
> This change ensures that:
> * the test node is not present before the test
> * the test runs to completion, including removing the test node
> * the test node is not left behind after the test
>
> Thanks.
Looks good to me.
test/jdk/java/util/prefs/AddNodeChangeListener.java line 63:
> 61: } finally {
> 62: // Make sure test node is not present after the test
> 63: clearPrefs();
If this call throws a `RuntimeException("Unable to clear...")`, it will
override the possible `RuntimeException("Failed")` in the `try` block, but I
think it is OK.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
On Tue, 22 Mar 2022 17:59:15 GMT, Brent Christian wrote: > Please review this change to the java/util/prefs/AddNodeChangeListener.jar > test. > > Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh > Preferences on each test run, MacOS does not seem to honor this, and still > stores prefs in Library/. > > This test has long suffered intermittent failures. 8255031 added some > debugging code; as of that change, the test fails fast as soon as an expected > change event is not received. In particular, if the expected event is not > received for adding the node, the test exits without removing the node. In > this way, on Mac, the node can get "stuck" in the preferences of the machine. > Future runs on the machine will already have the node, no node added change > event will be generated (because the node was already there), the test will > continue to fail without removing the node, etc. This matches observations on > some CI machines. > > This change ensures that: > * the test node is not present before the test > * the test runs to completion, including removing the test node > * the test node is not left behind after the test > > Thanks. I'm not a regular contributor in this area, so maybe wait to see if a second opinion is forthcoming, but otherwise the proposed changes look reasonable to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7908
RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
Please review this change to the java/util/prefs/AddNodeChangeListener.jar test. Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh Preferences on each test run, MacOS does not seem to honor this, and still stores prefs in Library/. This test has long suffered intermittent failures. 8255031 added some debugging code; as of that change, the test fails fast as soon as an expected change event is not received. In particular, if the expected event is not received for adding the node, the test exits without removing the node. In this way, on Mac, the node can get "stuck" in the preferences of the machine. Future runs on the machine will already have the node, no node added change event will be generated (because the node was already there), the test will continue to fail without removing the node, etc. This matches observations on some CI machines. This change ensures that: * the test node is not present before the test * the test runs to completion, including removing the test node * the test node is not left behind after the test Thanks. - Commit messages: - Ensure test node does not already exist, and is always removed Changes: https://git.openjdk.java.net/jdk/pull/7908/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7908=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283349 Stats: 70 lines in 1 file changed: 30 ins; 17 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/7908.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7908/head:pull/7908 PR: https://git.openjdk.java.net/jdk/pull/7908
