RFR: 8333403: Write a test to check various components events are triggered properly [v7]
Alexey Ivanov
aivanov at openjdk.org
Wed Sep 4 16:15:28 UTC 2024
On Mon, 5 Aug 2024 04:46:47 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 comments fixed
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 1:
> 1: /*
I suggest moving the test to `java/awt/Component` — you're testing the functionality of `Component` class and its subclasses.
It looks to me all the tests under `java/awt/event` should be moved to a particular component where the event is expected or not expected to be generated. So that these tests aren't missed when that component is modified and the developer runs tests for that specific component.
Yes, the developer is expected to run all the tests anyway, however, I would still expect all tests related to a component be under the component class folder.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 67:
> 65: private static volatile Dimension compSize;
> 66: private static final java.util.Collection<ComponentEvent> events =
> 67: Collections.synchronizedList(new ArrayList<ComponentEvent>());
Any reason why you use fully qualified `java.util.Collection` class name instead of importing it?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 67:
> 65: private static volatile Dimension compSize;
> 66: private static final java.util.Collection<ComponentEvent> events =
> 67: Collections.synchronizedList(new ArrayList<ComponentEvent>());
Suggestion:
private static final Collection<ComponentEvent> events =
Collections.synchronizedList(new ArrayList<>());
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 67:
> 65: private static volatile Dimension compSize;
> 66: private static final java.util.Collection<ComponentEvent> events =
> 67: Collections.synchronizedList(new ArrayList<ComponentEvent>());
I suggest re-organising the fields in another order and putting blank lines between groups:
private static final int DELAY = 500;
private static Frame frame;
private static Robot robot;
private static Component[] components;
private static volatile Point compAt;
private static volatile Dimension compSize;
private static volatile boolean componentHidden;
private static volatile boolean componentShown;
private static volatile boolean componentMoved;
private static volatile boolean componentResized;
private static final Collection<ComponentEvent> events =
Collections.synchronizedList(new ArrayList<ComponentEvent>());
This way it's more readable in my opinion.
The most common: `frame` and `robot`, then array of components, location to click on (see below as well), event flags and a list (collection) of generated events.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 157:
> 155: EventQueue.invokeAndWait(() -> {
> 156: compAt = components[9].getLocationOnScreen();
> 157: compSize = components[9].getSize();
Why did you change it from 0 to 9?
The number 9 looks like a magic number, it may change if anything changes in the `components` array.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 161:
> 159:
> 160: robot.mouseMove(compAt.x + compSize.width / 2,
> 161: compAt.y + compSize.height / 2);
Suggestion:
// Add a comment why you click a component
EventQueue.invokeAndWait(() -> {
Point location = components[9].getLocationOnScreen();
Dimension size = components[9].getSize();
centerPoint = new Point(location.x + size.width / 2,
location.y + size.height / 2);
});
robot.mouseMove(centerPoint.x, centerPoint.y);
You can calculate the center point where you click inside `invokeAndWait` and then assign the resulting value to `centerPoint` (rename `compAt` to `centerPoint` or another name). Remove `compSize`.
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);
Why do you click a component? You didn't answer the previous question.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 174:
> 172: robot.delay(DELAY);
> 173:
> 174: resetValues();
Suggestion:
System.out.println("Iconify frame");
resetValues();
Print a message what you're testing.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 177:
> 175: EventQueue.invokeAndWait(() -> frame.setExtendedState(Frame.ICONIFIED));
> 176:
> 177: robot.delay(DELAY);
Suggestion:
robot.waitForIdle();
robot.delay(DELAY);
Since changing the state of frame generates system events, it's better to add `waitForIdle()` to ensure all the system event are processed before you test the flags.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 183:
> 181: }
> 182:
> 183: resetValues();
Suggestion:
System.out.println("Deiconify frame");
resetValues();
Print a message.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 201:
> 199: System.err.println();
> 200: throw new RuntimeException("FAIL: " + errorMsg);
> 201: }
So you're collecting a list of events to print them if iconify or deiconify generates an unexpected list of events.
In all other cases `events` isn't used at all.
Given that all the generated events are printed as they're come, I see no reason why `events` is needed at all. By printing messages that you're about to test iconifying frame and deiconifying frame you separate the cases, the list of events will be printed between these messages.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 206:
> 204: throws InvocationTargetException, InterruptedException {
> 205:
> 206: Component currentComponent = components[i];
Suggestion:
private static void doTest(final Component currentComponent, boolean enable)
throws InvocationTargetException, InterruptedException {
You don't use the index for anything else but assigning the current component — pass it into the method. Replace any occurrences of `components[i]` with `currentComponent`, it'll be consistent.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 208:
> 206: Component currentComponent = components[i];
> 207: System.out
> 208: .println("Component " + currentComponent + "is enabled " + enable);
It is so uncommon to break `System.out.println` this way, these are usually kept together.
Anyway, I suggest splitting that into two lines: `toString` representation of a component is pretty long, therefore printing the `enabled` flag on its own line helps to see it.
Suggestion:
System.out.println("Component " + currentComponent);
System.out.println(" enabled " + enable);
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 305:
> 303: + "when size decreases for " + components[i].getClass());
> 304: }
> 305: }
Suggestion:
}
System.out.println("\n");
}
Printing two blank lines after a component is tested will help you parse the log. You'll be able to easily find where one portion of the test finishes and another starts.
-------------
PR Review: https://git.openjdk.org/jdk/pull/19521#pullrequestreview-2280441854
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744073977
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1743933493
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1743949637
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1743980292
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1743971465
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1743997273
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1743985969
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744051268
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744061638
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744051797
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744058260
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744028795
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744033326
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1744036241
More information about the client-libs-dev
mailing list