8341984: Use PassFailJFrame for TimeChangeButtonClickTest.java #21476
Conversation
|
👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into |
|
@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: 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 975 new commits pushed to the
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 |
|
@aivanov-jdk The following label will be automatically applied to this pull request:
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. |
Webrevs
|
| PassFailJFrame.builder() | ||
| .instructions(INSTRUCTIONS) | ||
| .rows(20) | ||
| .columns(40) |
There was a problem hiding this comment.
| .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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good for a workaround.
|
@aivanov-jdk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@aivanov-jdk This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
Refactor the test
javax/swing/JButton/TimeChangeButtonClickTest.javato use thePassFailJFrameframework 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
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21476/head:pull/21476$ git checkout pull/21476Update a local copy of the PR:
$ git checkout pull/21476$ git pull https://git.openjdk.org/jdk.git pull/21476/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21476View PR using the GUI difftool:
$ git pr show -t 21476Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21476.diff
Using Webrev
Link to Webrev Comment