On Tue, 27 Feb 2024 21:13:21 GMT, Alisen Chung <ach...@openjdk.org> wrote:
>> Root cause of the test failure was fixed with >> https://bugs.openjdk.org/browse/JDK-8316931, updating this test since the >> other fix also included a test update. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > update test from feedback Changes requested by aivanov (Reviewer). test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 68: > 66: " something is wrong the corresponding message is > displayed.\n" + > 67: " Repeat clicks several times. If no 'Test > FAILED' messages\n" + > 68: " are printed, press PASS button else FAIL > button."; My previous comment implies you should update the instructions to state that the test will _fail automatically_ if a failure condition detected. After repeating clicking several times, the tester can press the _Pass_ button. The tester should never click the _Fail_ button. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 71: > 69: > 70: PassFailJFrame.builder() > 71: .title("Event Message Display") Suggestion: .title("Dispose In ActionEvent Test") The name of the test is good enough if you want to set a custom title. What does "Event Message Display" mean? The instruction UI frame contain the instructions and Pass/Fail buttons. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 82: > 80: > 81: private static JFrame showFrameAndIcon() { > 82: JFrame frame = new JFrame("DisposeInActionEventTest"); Suggestion: JFrame frame = new JFrame("Event Message Display"); This is where events are displayed. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 88: > 86: textArea = new JTextArea(); > 87: spane.add(textArea); > 88: frame.getContentPane().add(spane); The code should look like this: frame.getContentPane().add(new JScrollPane(textArea)); test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 112: > 110: textArea.append(e + "\n"); > 111: e.printStackTrace(); > 112: throw new RuntimeException(); If you throw another exception after catching, pass the original exception as the `cause` parameter. Since this is a manual test, you should use `forceFail` to exit the test gracefully: Suggestion: e.printStackTrace(); PassFailJFrame.forceFail("Exception caught: " + e.getMessage()); Adding a message to `textArea` makes no sense, the tester won't be able to see it — the test shuts down right after you do it. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 122: > 120: textArea.append(e + "\n"); > 121: e.printStackTrace(); > 122: throw new RuntimeException("Test failed."); As above. ------------- PR Review: https://git.openjdk.org/jdk/pull/17838#pullrequestreview-1905984379 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1506066597 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1505784352 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1506067515 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1505786130 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1505790776 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1505791483
