On Mon, 26 Feb 2024 20:32:54 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: > > remove system property set The subject of the bug needs to be updated. The root cause has been resolved under another bug. Here, you update the test… because it uses `Applet`. You don't actually fix the test to make it pass. Perhaps even, create a _new bug_ with clear description and close [JDK-8325097](https://bugs.openjdk.org/browse/JDK-8325097) as duplicate of [JDK-8316931](https://bugs.openjdk.org/browse/JDK-8316931) because JDK-8316931 fixed both failures. For this reason, JDK-8316931 deserves being in the `@bug` tag in both tests: `DisposeInActionEventTest` and `ShowAfterDisposeTest`. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 38: > 36: /* > 37: * @test > 38: * @bug 6299866 Does this test serve a regression test for [JDK-8316931](https://bugs.openjdk.org/browse/JDK-8316931)? Then 8316931 should be added to `@bug` in this test as well as in `test/jdk/java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.java`. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 42: > 40: * handler of action event caused by clicking on this icon. > 41: * @library ../../regtesthelpers /test/lib > 42: * @build Sysout jtreg.SkippedException Suggestion: * @build PassFailJFrame jtreg.SkippedException `Sysout` isn't used in the test. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 60: > 58: " (also called Taskbar Status Area on MS Windows, > Notification\n" + > 59: " Area on Gnome or System Tray on KDE) is > visible.\n\n" + > 60: clickInstruction + " the button on the tray icon > to trigger the\n" + _“Right-click the button”_ or _“Double-left click the button”_ doesn't make sense to me. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 67: > 65: " something is wrong the corresponding message is > displayed below.\n" + > 66: " Repeat clicks several times. If no 'Test > FAILED' messages\n" + > 67: " are printed, press PASS button else FAIL > button."; It's not displayed below, it's to the right, isn't it? Fail the test automatically if you can detect the failure. You may show a message to the tester about the failure before shutting down the test. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 70: > 68: > 69: PassFailJFrame.builder() > 70: .title("Test Instructions Frame") More specific title would be better. Otherwise, you can remove `.title()` to get the same default title. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 77: > 75: .testUI(DisposeInActionEventTest::showFrameAndIcon) > 76: .build() > 77: .awaitAndCheck(); I propose removing the icon from the system tray in a `finally` block. Otherwise, the test doesn't exit if you run it without jtreg. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 85: > 83: > 84: textArea = new JTextArea(); > 85: frame.getContentPane().add(textArea); Suggestion: frame.getContentPane().add(new JScrollPane(textArea)); Put the text area into a scroll pane to allow viewing all text even it doesn't fit. And the messages with `ActionEvent` do not fit. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 100: > 98: trayIcon.setImageAutoSize(true); > 99: trayIcon.addActionListener(ev -> { > 100: textArea.append(ev.toString() + "\n"); Suggestion: textArea.append(ev + "\n"); `.toString()` is redundant. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 109: > 107: textArea.append(e.toString() + "\n"); > 108: textArea.append("!!! The test couldn't be performed > !!!\\n"); > 109: } Fail the test? Print the stack trace of the exception with `e.printStackTrace()` — it would help analysing the failure. test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 118: > 116: textArea.append(e.toString() + "\n"); > 117: textArea.append("!!! The test couldn't be performed !!!\n"); > 118: } Also fail the test right away. ------------- Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17838#pullrequestreview-1903177412 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504041086 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504034589 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504044049 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504046153 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504047731 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504050445 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504048925 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504052584 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504054219 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1504055513
