On Fri, 15 Nov 2024 21:04:10 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>> Alisen Chung has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - EOF newline ActionEventTest
>> - revert ActionEventTest
>
> test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 49:
>
>> 47: private JInternalFrame iFrame;
>> 48: private static TestTitlePane testTitlePane;
>> 49: private boolean passed;
>
> Suggestion:
>
> private static volatile boolean passed;
Not required… because of `invokeAndWait`, yet it's a common practice to mark
with `volatile`.
> test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 54:
>
>> 52: public static void main(String[] args) throws Exception {
>> 53: UIManager.setLookAndFeel(
>> 54: new
>> com.sun.java.swing.plaf.windows.WindowsClassicLookAndFeel());
>
> Is fully qualified name required? I think we can shorten it by adding
> required imports.
Perhaps, it's better to use `getSystemLookAndFeelClassName()`.
> test/jdk/javax/swing/JInternalFrame/6725409/bug6725409.java line 138:
>
>> 136:
>> 137: private static void sync() {
>> 138: robot.waitForIdle();
>
> Since sync() is called after UI setup better to add `robot.delay(500)` for
> stability.
It don't think it's necessary or required. The test does not access UI at all,
it just verifies the title of menu items.
Adding `delay(500)` to `sync` makes the test run longer… without a reason.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1847163245
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1847150967
PR Review Comment: https://git.openjdk.org/jdk/pull/20846#discussion_r1847165866
