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

[Alexey Ivanov]

On Tue, 17 Sep 2024 16:19:27 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: Whitespace Fixed

I am still for moving the test into `java/awt/Component` folder.

test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 50:

> 48:  * @bug 8333403
> 49:  * @summary Test performs various operations to check components events are triggered properly.
> 50:  * @run main ComponentEventTest

Add the library and build tags to use the `Platform` class.

Suggestion:

 * @summary Test performs various operations to check components events are triggered properly.
 * @library /test/lib
 * @build jdk.test.lib.Platform
 * @run main ComponentEventTest


And don't forget to add `jdk.test.lib.Platform` to the list of imports.

test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 149:

> 147:     private static void doTest()
> 148:         throws InvocationTargetException, InterruptedException {
> 149:         // Click on the Frame to ensure its gain Focus

Suggestion:

        // Click the frame to ensure it gains focus

I see no reason why you capitalise “frame” and “focus”.

test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 161:

> 159:         robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 160: 
> 161:         robot.delay(DELAY);

Does it make sense to move this part into a new method `clickFrame`?

Does anyone else have any opinions on this?

test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 171:

> 169:         robot.delay(DELAY);
> 170: 
> 171:         System.out.println("Iconify frame");

Similarly, iconify and deiconify could be moved into a separate method. Shorter, logically complete pieces improve the code quality and the overall readability of the code.

test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 196:

> 194:          * the events from the native system.
> 195:          * Please check the  JDK-6754618
> 196:          */

Suggestion:

        /*
         * Because of the different behavior between MS Windows and other OS,
         * we receive native events WM_SIZE and WM_MOVE on Windows
         * when the frame state changes from iconified to normal.
         * AWT sends these events to components when it receives
         * the events from the native system.
         * See JDK-6754618 for more information.
         */

test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 208:

> 206:             throw new RuntimeException(
> 207:                 "ComponentEvent triggered when frame set to normal");
> 208:         }

I suggest using the `Platform` class to determine whether the test runs on Windows or not and splitting the common condition and platform-dependent conditions:


        if (componentShown || componentHidden) {
            throw new RuntimeException(
                    "FAIL: componentShown or componentHidden triggered "
                    + "when frame set to normal");
        }

        if (Platform.isWindows() && (!componentMoved || !componentResized)) {
            throw new RuntimeException(
                    "FAIL: componentMoved or componentResized wasn't triggered "
                    + "when frame set to normal");
        }
        if (!Platform.isWindows() && (componentMoved || componentResized)) {
            throw new RuntimeException(
                "FAIL: componentMoved or componentResized triggered "
                + "when frame set to normal");
        }


This allows you to report a clearer error message.

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

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19521#pullrequestreview-2310234320
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763592072
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763532753
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763582314
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763583953
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763545672
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763563500