8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java #21476

aivanov-jdk · 2024-10-11T18:47:47Z

Refactor the test javax/swing/JButton/TimeChangeButtonClickTest.java to use the PassFailJFrame framework to handle the tester's decision on whether the test passes or fails.

I reformatted the instructions for performing the test into HTML.

I preserved the test UI panel which contains the button to press and the label with button press counter.

The updated test is shorter by nearly 100 lines.

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-8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java (Bug - P4)

Reviewers

Harshitha Onkar (@honkar-jdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21476

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-11T18:48:26Z

👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into master 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-10-11T18:49:15Z

@aivanov-jdk 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:

8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java

Reviewed-by: honkar

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 22 new commits pushed to the master branch:

bd62644: 8341924: Improve error message with structurally malformed Code array
a2c7752: 8317356: Fix missing null checks in the ClassFile API
6071346: 8339879: Open some dialog awt tests
a8a8b2d: 8341831: PhaseCFG::insert_anti_dependences asserts with "no loads"
9e262df: 8342002: sun/security/tools/keytool/GenKeyPairSigner.java failed due to missing certificate output
f56a154: 8341881: [REDO] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
ba3774d: 8341637: java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
1581508: 8335091: NMT: VMATree reserve_mapping and commit_mapping APIs need MEMFLAGS while un/-committing API has no MEMFLAGS arg
b20c5c7: 8341967: Unify os::current_file_offset and os::seek_to_file_offset across posix platforms
dcac4b0: 8341471: Reversed field layout caused by unstable sorting
... and 12 more: https://git.openjdk.org/jdk/compare/7276a1bec0d90f63e9e433fdcdfd6564b70dc9bb...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-10-11T18:49:54Z

@aivanov-jdk The following label will be automatically applied to this pull request:

client

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-10-11T18:52:25Z

Webrevs

01: Full - Incremental (1d5b62d4)
00: Full (b71d5b90)

test/jdk/javax/swing/JButton/TimeChangeButtonClickTest.java

honkar-jdk

LGTM other than minor suggestion on default timeout.

honkar-jdk · 2024-10-14T18:54:44Z

test/jdk/javax/swing/JButton/TimeChangeButtonClickTest.java

+        PassFailJFrame.builder()
+                      .instructions(INSTRUCTIONS)
+                      .rows(20)
+                      .columns(40)


Suggested change

.columns(40)

.columns(40)

.testTimeOut(10)

We do have a pause timer option but I think a timeout of 8-10 mins is better than the default of 5 mins since it allows more time for the user to change the time settings.

Actually, I think what is happening is that when the time is set forward an hour (+2 hours forward from 1 hour behind as in instructions) the test will automatically time out because it thinks an hour has passed. I thought initially it was me being slow on changing the time settings but even if you do everything quickly the test always fails exactly when you set the time forward.

So I think with the way the timeout is implemented the test is broken with PassFailJFrame.

What an .. interesting .. test. No idea what this does to the jtreg test harness too.
FWIW I think this test should be deleted rather than trying to make it nicer.

Actually, I think what is happening is that when the time is set forward an hour (+2 hours forward from 1 hour behind as in instructions) the test will automatically time out because it thinks an hour has passed. I thought initially it was me being slow on changing the time settings but even if you do everything quickly the test always fails exactly when you set the time forward.

Good catch. I hadn't noticed PFJ timeout.

Indeed, the test does not play nicely with timeout in PassFailJFrame.

The workaround could be to pause the timeout. In addition to that, we could set the timeout to 15 minutes. If the user changes time not -1 / +2 hours but rather -5 / +10 minutes, it should be enough.

What an .. interesting .. test. No idea what this does to the jtreg test harness too.

The jtreg test harness turns off its timeout for manual tests, as far as I understand. By the time, the test finishes, the time on the system should be correct again.

FWIW I think this test should be deleted rather than trying to make it nicer.

Another option could be to mark the test with @ignore so that it's not run regularly. The test could still be useful; even though changing time isn't common…

I wonder if going to/from DST reproduces the problem in JDK-7096375 for which the test was written.

The workaround could be to pause the timeout. In addition to that, we could set the timeout to 15 minutes. If the user changes time not -1 / +2 hours but rather -5 / +10 minutes, it should be enough.

The workaround sounds good and may be also include pause timer in the test instructions as below.

Pause the timer and reduce the system time to 10 mins less than current time. ...... ..... Pause the timer and increase the system time by adding 10 mins to the current time.

Yes I agree, retaining the test might be a good idea since it was created for -
JDK-7096375.

Sounds good for a workaround.

8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java

b71d5b9

openjdk bot added the rfr Pull request is ready for review label Oct 11, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Oct 11, 2024

kumarabhi006 reviewed Oct 14, 2024

View reviewed changes

test/jdk/javax/swing/JButton/TimeChangeButtonClickTest.java Outdated Show resolved Hide resolved

test/jdk/javax/swing/JButton/TimeChangeButtonClickTest.java Outdated Show resolved Hide resolved

aivanov-jdk added 2 commits October 14, 2024 19:31

Add @library and @build jtreg tags

89dab3b

Rename instructions -> INSTRUCTIONS

1d5b62d

honkar-jdk approved these changes Oct 14, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java #21476

8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java #21476

aivanov-jdk commented Oct 11, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024 •

edited

Loading

openjdk bot commented Oct 11, 2024

mlbridge bot commented Oct 11, 2024 •

edited

Loading

honkar-jdk left a comment

honkar-jdk Oct 14, 2024

alisenchung Oct 15, 2024

prrace Oct 15, 2024 •

edited

Loading

honkar-jdk Oct 15, 2024

aivanov-jdk Oct 18, 2024

honkar-jdk Oct 18, 2024 •

edited

Loading

kumarabhi006 Oct 21, 2024

8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java #21476

Are you sure you want to change the base?

8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java #21476

Conversation

aivanov-jdk commented Oct 11, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024 • edited Loading

openjdk bot commented Oct 11, 2024

mlbridge bot commented Oct 11, 2024 • edited Loading

Webrevs

honkar-jdk left a comment

Choose a reason for hiding this comment

honkar-jdk Oct 14, 2024

Choose a reason for hiding this comment

alisenchung Oct 15, 2024

Choose a reason for hiding this comment

prrace Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

honkar-jdk Oct 15, 2024

Choose a reason for hiding this comment

aivanov-jdk Oct 18, 2024

Choose a reason for hiding this comment

honkar-jdk Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

kumarabhi006 Oct 21, 2024

Choose a reason for hiding this comment

aivanov-jdk commented Oct 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 11, 2024 •

edited

Loading

mlbridge bot commented Oct 11, 2024 •

edited

Loading

prrace Oct 15, 2024 •

edited

Loading

honkar-jdk Oct 18, 2024 •

edited

Loading