RFR: 8316388: Opensource five Swing component related regression tests
Alexey Ivanov
aivanov at openjdk.org
Wed Mar 13 16:57:21 UTC 2024
On Wed, 13 Mar 2024 06:49:16 GMT, Alexander Zuev <kizune at 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
More information about the client-libs-dev
mailing list