RFR: 8333403: Write a test to check various components events are triggered properly [v6]

[Alexey Ivanov]

On Fri, 12 Jul 2024 08:40:13 GMT, Ravi Gupta <rgupta at openjdk.org> wrote:

>> This testcase checks for the following assertions for Component events:
>> 
>> 1. When components are resized, moved, hidden and shown the respective events are triggered.
>> 2. When the components are hidden/disabled also,the component events like resized/moved are triggered.
>> 3. When a hidden component is hidden again, or a visible component is shown again, the events should not be fired.
>> 4. When a window is minimized/restored then hidden and shown component events should be triggered.
>> 
>> Testing:
>> Tested using Mach5(20 times per platform) in macos,linux and windows and got all pass.
>
> Ravi Gupta has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8333403: Review commnets fixed

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 60:

> 58:     private static Robot robot;
> 59:     private static Component[] components;
> 60:     private volatile static boolean componentHidden;

Missorted modifiers: `static` goes before `volatile`.

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 67:

> 65:     private volatile static Dimension compSize;
> 66:     private volatile static java.util.List<ComponentEvent> events =
> 67:         Collections.synchronizedList(new ArrayList<ComponentEvent>());

Suggestion:

    private static final java.util.List<ComponentEvent> events =
        Collections.synchronizedList(new ArrayList<ComponentEvent>());


It should be `final` rather than `volatile`. The reason why you use `java.util.List` is because `List` refers to `java.awt.List` in this test, isn't it?

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 164:

> 162: 
> 163:         robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
> 164:         robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);

Do you click the panel to ensure the frame has focus?

Does the test fail if the frame fails to be the active frame for whatever reason? (It should always be the active frame / window.)

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 177:

> 175:         EventQueue.invokeAndWait(() -> {
> 176:             frame.setExtendedState(Frame.ICONIFIED);
> 177:         });

Suggestion:

        EventQueue.invokeAndWait(() -> frame.setExtendedState(Frame.ICONIFIED));

You can use lambda expression instead of lambda statement.

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 188:

> 186:         EventQueue.invokeAndWait(() -> {
> 187:             frame.setExtendedState(Frame.NORMAL);
> 188:         });

Suggestion:

        EventQueue.invokeAndWait(() -> frame.setExtendedState(Frame.NORMAL));

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 191:

> 189: 
> 190:         robot.delay(DELAY);
> 191:         if (componentShown || componentHidden) {

Is it possible that any other event is triggered? Or do you expect other events?

When you inconify the frame, you verify of all the event flags but you verify only two flags here.

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 200:

> 198:         for (int j = 0; j < events.size(); j++) {
> 199:             System.err.print(events.get(j) + "; ");
> 200:         }

Suggestion:

        for (ComponentEvent event : events) {
            System.err.print(event + "; ");
        }

You don't use the index for anything else but getting an element, you can use the improved `for`-loop.

If you do, you can replace the type of `events` from `java.util.List` to `java.util.Collection`.

*Strictly*, you should iterate over `events` while holding the lock to the `events` object, even though there's no strong need for it since the list of events can't be modified.


        synchronized (events) {
            for (ComponentEvent event : events) {
                System.err.print(event + "; ");
            }
        }

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 206:

> 204: 
> 205:     private static void doTest(int i, boolean enable)
> 206:         throws InvocationTargetException, InterruptedException {

I think you should print the current value of the `enabled` flag. If the test fails for any component, you want to know if it fails when components are enabled or disabled.

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 290:

> 288:         if (!componentResized) {
> 289:             throw new RuntimeException("FAIL: ComponentResized not triggered "
> 290:                 + "when size increase for " + components[i].getClass());

Suggestion:

                + "when size increases for " + components[i].getClass());

test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 303:

> 301:         if (!componentResized) {
> 302:             throw new RuntimeException("FAIL: ComponentResized not triggered "
> 303:                 + "when size decrease for " + components[i].getClass());

Suggestion:

                + "when size decreases for " + components[i].getClass());

-------------

PR Review: https://git.openjdk.org/jdk/pull/19521#pullrequestreview-2197535744
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690289860
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690292879
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690317573
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690306818
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690307166
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690308601
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690297666
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690319279
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690315534
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1690315668