On Wed, 13 Mar 2024 06:49:16 GMT, Alexander Zuev <kiz...@openjdk.org> wrote:
> Cleaned up five more tests. > > Continuation of https://github.com/openjdk/jdk/pull/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. Changes requested by aivanov (Reviewer). test/jdk/javax/swing/JDesktopPane/bug4773378.java line 53: > 51: > 52: Robot robot; > 53: volatile boolean frameActivated = false; Suggestion: private final CountDownLatch frameActivated = new CountDownLatch(1); test/jdk/javax/swing/JDesktopPane/bug4773378.java line 71: > 69: frameActivated = true; > 70: bug4773378.this.notifyAll(); > 71: } Suggestion: frameActivated.countDown(); test/jdk/javax/swing/JDesktopPane/bug4773378.java line 92: > 90: } catch (PropertyVetoException e) { > 91: e.printStackTrace(); > 92: } Suggestion: try { jif.setSelected(true); } catch (PropertyVetoException e) { throw new RuntimeException(e); } Fail the test if an unexpected is thrown. Can `jif.setSelected(true)` be called in the setup code? test/jdk/javax/swing/JDesktopPane/bug4773378.java line 99: > 97: bug4773378.this.wait(); > 98: } > 99: } Suggestion: frameActivated.await(); Using `CountDownLatch` is so much cleaner. test/jdk/javax/swing/JDesktopPane/bug4773378.java line 107: > 105: robot.keyRelease(KeyEvent.VK_CONTROL); > 106: > 107: Thread.sleep(2000); Suggestion: robot.waitForIdle(); Is it really necessary to wait for 2 seconds before shutting down the test. According to [JDK-4773378](https://bugs.openjdk.org/browse/JDK-4773378), `NullPointerException` was thrown when <kbd>Ctrl</kbd>+<kbd>F6</kbd> 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. test/jdk/javax/swing/JDesktopPane/bug4773378.java line 117: > 115: } > 116: > 117: class MyDesktopManager extends DefaultDesktopManager { Suggestion: private static class MyDesktopManager extends DefaultDesktopManager { test/jdk/javax/swing/JEditorPane/bug4325606.java line 80: > 78: robo = new Robot(); > 79: } catch (AWTException e) { > 80: throw new RuntimeException("Robot could not be created"); Suggestion: throw new RuntimeException("Robot could not be created", e); Preserve the original exception, which would be very helpful for debugging if it ever occurs. test/jdk/javax/swing/JEditorPane/bug4325606.java line 84: > 82: robo.setAutoDelay(100); > 83: robo.delay(1000); > 84: Point p = frame.getLocationOnScreen(); Technically, `getLocationOnScreen` should be called on EDT. test/jdk/javax/swing/JEditorPane/bug4325606.java line 101: > 99: } catch (BadLocationException blex) { > 100: passed = false; > 101: } Suggestion: } catch (BadLocationException blex) { throw new RuntimeException("Test failed", blex); } Throw exception preserving the original exception which will help analysing the failure? test/jdk/javax/swing/JEditorPane/bug4325606.java line 120: > 118: if (!b.passed) { > 119: throw new RuntimeException("Test failed."); > 120: } Suggestion: 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. test/jdk/javax/swing/JEditorPane/bug4694598.java line 64: > 62: } catch (IOException ioe){ > 63: throw new RuntimeException("Could not create html file to > embed", ioe); > 64: } Move creating the file to `main` method before setting up GUI and let `IOException` escape from main. test/jdk/javax/swing/JEditorPane/bug4694598.java line 74: > 72: String html = "<HTML> <BODY>" + > 73: "<FRAMESET cols=\"100%\">" + > 74: "<FRAME src=\"" + frameContentUrl + "\">" + Suggestion: "<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. ------------- PR Review: https://git.openjdk.org/jdk/pull/18259#pullrequestreview-1934620670 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523563210 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523564258 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523549703 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523565594 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523573470 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523575719 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523578951 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523587677 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523582352 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523586920 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523592475 PR Review Comment: https://git.openjdk.org/jdk/pull/18259#discussion_r1523611406
