8316388: Opensource five Swing component related regression tests #18259
Conversation
Cleaned up five more tests.
|
👋 Welcome back kizune! A progress list of the required criteria for merging this PR into |
|
@azuev-java 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
|
| JInternalFrame jif; | ||
|
|
||
| Robot robot; | ||
| volatile boolean frameActivated = false; |
There was a problem hiding this comment.
| volatile boolean frameActivated = false; | |
| private final CountDownLatch frameActivated = new CountDownLatch(1); |
| synchronized (bug4773378.this) { | ||
| frameActivated = true; | ||
| bug4773378.this.notifyAll(); | ||
| } |
There was a problem hiding this comment.
| synchronized (bug4773378.this) { | |
| frameActivated = true; | |
| bug4773378.this.notifyAll(); | |
| } | |
| frameActivated.countDown(); |
| synchronized (this) { | ||
| while (!frameActivated) { | ||
| bug4773378.this.wait(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| synchronized (this) { | |
| while (!frameActivated) { | |
| bug4773378.this.wait(); | |
| } | |
| } | |
| frameActivated.await(); |
Using CountDownLatch is so much cleaner.
There was a problem hiding this comment.
You like your countdown latches :) Ok, i will re-write this test with CDL.
There was a problem hiding this comment.
Because 1 line of code replaces 4 lines of code and communicates what's happening much clearer than a synchronized block with a flag.
| robot.keyRelease(KeyEvent.VK_F6); | ||
| robot.keyRelease(KeyEvent.VK_CONTROL); | ||
|
|
||
| Thread.sleep(2000); |
There was a problem hiding this comment.
| Thread.sleep(2000); | |
| robot.waitForIdle(); |
Is it really necessary to wait for 2 seconds before shutting down the test.
According to JDK-4773378, NullPointerException was thrown when Ctrl+F6 was pressed. The waitForIdle method doesn't return until the event queue is empty which implies the keyboard events are handled. It saves nearly 2 seconds.
| } catch (BadLocationException blex) { | ||
| passed = false; | ||
| } |
There was a problem hiding this comment.
| } catch (BadLocationException blex) { | |
| passed = false; | |
| } | |
| } catch (BadLocationException blex) { | |
| throw new RuntimeException("Test failed", blex); | |
| } |
Throw exception preserving the original exception which will help analysing the failure?
| safeSleep(3000); | ||
| if (!b.passed) { | ||
| throw new RuntimeException("Test failed."); | ||
| } |
There was a problem hiding this comment.
| safeSleep(3000); | |
| if (!b.passed) { | |
| throw new RuntimeException("Test failed."); | |
| } | |
| robot.waitForIdle(); |
Wait until all events are processed. If test fails, it throws an exception on EDT; otherwise, the test is finished as soon as the event queue is empty. No need to waste 3 seconds.
| } | ||
| robo.setAutoDelay(100); | ||
| robo.delay(1000); | ||
| Point p = frame.getLocationOnScreen(); |
There was a problem hiding this comment.
Technically, getLocationOnScreen should be called on EDT.
There was a problem hiding this comment.
It happens in the windowOpened method of the WindowAdapter. If window events arrive not on EDT then we have a whole other slew of problems :)
| try (Writer writer = Files.newBufferedWriter(frameContentFile)) { | ||
| writer.write(frameContentString); | ||
| } catch (IOException ioe){ | ||
| throw new RuntimeException("Could not create html file to embed", ioe); | ||
| } |
There was a problem hiding this comment.
Move creating the file to main method before setting up GUI and let IOException escape from main.
There was a problem hiding this comment.
This is try-with-resources so if i will do it in main i will have to add synchronizing and closing of writer which is a strange trade-off so i would have to do try block anyways.
There was a problem hiding this comment.
Why would you need synchronisation?
try (Writer writer = Files.newBufferedWriter(frameContentFile)) {
writer.write(frameContentString);
}would write out the contents of the file and close the file handle. In setupGui, you would still use frameContentFile object only which is final and immutable, therefore it's thread-safe.
There was a problem hiding this comment.
And that's pretty much what i do here - except for the exception handling. I just do not see a reason to move it to main, it makes no difference except now the HTML creation will be split and harder to understand.
| jep.setContentType("text/html"); | ||
| String html = "<HTML> <BODY>" + | ||
| "<FRAMESET cols=\"100%\">" + | ||
| "<FRAME src=\"" + frameContentUrl + "\">" + |
There was a problem hiding this comment.
| "<FRAME src=\"" + frameContentUrl + "\">" + | |
| "<FRAME src=\"" + frameContentFile.toUri()+ "\">" + |
Isn't it enough? Alternatively, "file:/" + frameContentFile.toAbsolutePath() produces the same result as frameContentFile.toUri().toURL().
Another option is to convert the Path to URL in the main method before calling setupGUI and remove try-catch blocks.
There was a problem hiding this comment.
Again, i do not mind the try/catch block and using URI instead of URL gives different result and i am not sure the original bug would be reproducible with URI which will make it a strange case of the regression test.
There was a problem hiding this comment.
Okay.
Without the try-catch blocks, the code is simpler… Ensuring the updated test still reproduces the original issue is always a good thing to do, but running the updated with an old version of Java often requires modifying the source code again.
|
@azuev-java 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 58 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 |
Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
| } | ||
| robo.setAutoDelay(100); | ||
| robo.delay(1000); | ||
| Point p = frame.getLocationOnScreen(); |
Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
|
/integrate |
|
Going to push as commit c05f8c7.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@azuev-java Pushed as commit c05f8c7. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Cleaned up five more tests.
Continuation of #18184
Unfortunately one of the commits rendered the whole PR invalid so i closed it and restarting it here.
All comments from the previous review are addressed.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18259/head:pull/18259$ git checkout pull/18259Update a local copy of the PR:
$ git checkout pull/18259$ git pull https://git.openjdk.org/jdk.git pull/18259/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18259View PR using the GUI difftool:
$ git pr show -t 18259Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18259.diff
Webrev
Link to Webrev Comment