On Tue, 22 Oct 2024 20:29:45 GMT, Alisen Chung <ach...@openjdk.org> wrote:
>> Cleaning up tests building ExtendedRobot that shouldn't be.
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update tests
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/TrayIcon/ActionEventTest/ActionEventTest.java line 30:
> 28: * @summary Verify that ActionEvent is received with correct modifiers
> set.
> 29: * @modules java.desktop/java.awt:open java.desktop/java.awt.peer
> 30: * @library /lib/client ../ /java/awt/patchlib
Is anything else but `ExtendedRobot` used from `/lib/client`? If not, remove it.
test/jdk/java/awt/TrayIcon/ActionEventTest/ActionEventTest.java line 54:
> 52: if (!SystemTray.isSupported()) {
> 53: System.out.println("SystemTray not supported on the
> platform." +
> 54: " Marking the test passed.");
Perhaps, throwing `jtreg.SkippedException` is a better option.
Now skipped tests are distinguished from passed ones.
test/jdk/java/awt/TrayIcon/ActionEventTest/ActionEventTest.java line 65:
> 63: "all icons and notifications on the
> taskbar\" true " +
> 64: "to avoid this problem.\nOR change
> behavior only for " +
> 65: "Java SE tray icon and rerun test.");
~~I'd rather keep them aligned as they are — easier to read.~~
Shall we remove this at all?
The test is automatic, isn't it? No one will see these messages unless the test
fails. [The
instructions](https://wiki.openjdk.org/display/ClientLibs/Automated+client+GUI+testing+system+set+up+requirements#AutomatedclientGUItestingsystemsetuprequirements-Windows-SpecificSystemsetupnotes)
for setting up Windows hosts to run client tests include:
> Show all icons and notifications in taskbar and don't hide them
> Right Click on Taskbar → Properties → (Notification area)
> Click Customize → Enable checkbox "Always show all icons and notifications on
> the taskbar"
test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 31:
> 29: * @summary Checks that JInternalFrame's system menu
> 30: * can be localized during run-time
> 31: * @library /lib/client/
Does the test use anything except `ExtendedRobot`? If not, remove `@library`.
test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 57:
> 55: UIManager.setLookAndFeel(
> 56: new
> com.sun.java.swing.plaf.windows.WindowsClassicLookAndFeel());
> 57: } catch (UnsupportedLookAndFeelException e) {
I would let the exception fail the test if it's run on anything but Windows.
The test has `@requires (os.family == "windows")`.
test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 64:
> 62: final bug6725409 bug6725409 = new bug6725409();
> 63: try {
> 64: SwingUtilities.invokeAndWait(() -> bug6725409.setupUIStep1());
Suggestion:
SwingUtilities.invokeAndWait(bug6725409::setupUIStep1);
Since you're still updating these lines, I'm for using method references (in
all the cases).
test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 86:
> 84: JDesktopPane desktop = new JDesktopPane();
> 85: iFrame = new JInternalFrame("Internal Frame",
> 86: true, true, true, true);
The line fits in 80 columns, so I'd rather keep it untouched.
test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 151:
> 149: try {
> 150: Robot robot = new Robot();
> 151: return robot;
Suggestion:
return new Robot();
Can be inlined?
test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 154:
> 152: } catch (Exception ex) {
> 153: // pass exception
> 154: }
This is wrong — if the test can't create `Robot`, let it fail right away.
Suggestion:
} catch (Exception ex) {
throw new Error("Can't create Robot", ex);
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/20846#pullrequestreview-2392728149
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815129228
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815075164
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815070228
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815116294
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815091498
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815093034
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815097951
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815109763
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1815106557
