RFR: JDK-8340555 : Open source DnD tests - Set4 [v6]
Alexey Ivanov
aivanov at openjdk.org
Wed Oct 2 15:59:46 UTC 2024
On Tue, 1 Oct 2024 18:59:22 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:
>> Following tests are added as part of this PR:
>>
>> 1. /java/awt/dnd/DnDHTMLToOutlookTest/DnDHTMLToOutlookTest.java - **Manual PassFailJFrame (PFJ) test**, problemlisted on all platforms.
>>
>> 2. java/awt/dnd/DragSourceMotionListenerTest.java - **Automated test**, problemlisted on windows
>>
>> 3. /java/awt/dnd/DragToAnotherScreenTest.java - **Manual PFJ test**
>>
>> 4. /java/awt/dnd/RejectDragTest.java - **Automated test**, problemlisted on macOS
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
>
> review comment fixes
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 73:
> 71: private static final Panel source = new TestPanel();
> 72: private static final Panel target = new TestPanel();
> 73: private static final DragSource ds = DragSource.getDefaultDragSource();
Both `source` and `target` are created on the main thread when the `DragSourceMotionListenerTest` class is initialised, before the `main` method is called.
Is it intended?
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 152:
> 150: srcPoint = source.getLocationOnScreen();
> 151: d[0] = source.getSize();
> 152: srcPoint.translate(d[0].width / 2, d[0].height / 2);
I meant local variable inside the lambda expressions rather than in `main`.
Suggestion:
srcPoint = source.getLocationOnScreen();
Dimension d = source.getSize();
srcPoint.translate(d[0].width / 2, d[0].height / 2);
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 152:
> 150: srcPoint = source.getLocationOnScreen();
> 151: d[0] = source.getSize();
> 152: srcPoint.translate(d[0].width / 2, d[0].height / 2);
The logic hasn't changed. It should rather look like I suggested in [my previous comment](https://github.com/openjdk/jdk/pull/21213#discussion_r1781127928):
Suggestion:
Point p = source.getLocationOnScreen();
Dimension d = source.getSize();
p.translate(d.width / 2, d.height / 2);
srcPoint = p;
Otherwise, the `volatile` modifier does not make sense. Prepare the object so that all its field have the correct values, then assign the reference to the object to the volatile field.
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 164:
> 162: d[0] = frame.getSize();
> 163: dstOutsidePoint.translate(3 * d[0].width / 2, d[0].height / 2);
> 164: testPoint1.setLocation(dstOutsidePoint);
The same issue as above.
Suggestion:
Point p = frame.getLocationOnScreen();
Dimension d = frame.getSize();
p.translate(3 * d.width / 2, d.height / 2);
dstOutsidePoint = p;
testPoint1.setLocation(p);
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 172:
> 170: d[0] = target.getSize();
> 171: dstInsidePoint.translate(d[0].width / 2, d[0].height / 2);
> 172: testPoint2.setLocation(dstInsidePoint);
The same issue:
Suggestion:
Point p = target.getLocationOnScreen();
Dimension d = target.getSize();
p.translate(d.width / 2, d.height / 2);
dstInsidePoint = p;
testPoint2.setLocation(p);
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 202:
> 200: robot.delay(1000);
> 201:
> 202: if (!passedTest1) {
The current lines 202–216 need to be indented by four spaces.
test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 235:
> 233: boolean pointInComponent(Robot robot, Point p, Component comp) throws Exception {
> 234: robot.waitForIdle();
> 235: reset();
The `reset` method is called only once from this place. I suggest assigning `clickedComponent = null` right here.
Does it look clearer? You don't have to look into the body of another method.
test/jdk/java/awt/dnd/DragToAnotherScreenTest.java line 137:
> 135: "getTransferData was successful",
> 136: "Test Passed", JOptionPane.PLAIN_MESSAGE);
> 137: PassFailJFrame.forcePass();
In this case, I would rather not call `forcePass` — the tester could repeat the drag… after moving the window for example.
I'm not insisting though.
test/jdk/java/awt/dnd/DragToAnotherScreenTest.java line 140:
> 138: } catch (Exception e) {
> 139: dtde.dropComplete(false);
> 140: PassFailJFrame.log("getTransferData() Failed: " + e);
You may still want to print the stack trace by `e.printStackTrace()` so that this information is available to failure analysis.
test/jdk/java/awt/dnd/DragToAnotherScreenTest.java line 144:
> 142: "getTransferData() Failed",
> 143: "Test Failed", JOptionPane.ERROR_MESSAGE);
> 144: PassFailJFrame.forceFail();
Provide the reason:
Suggestion:
PassFailJFrame.forceFail("getTransferData() Failed");
test/jdk/java/awt/dnd/DragToAnotherScreenTest.java line 153:
> 151: "stringFlavor is not supported by Transferable",
> 152: "Test Failed", JOptionPane.ERROR_MESSAGE);
> 153: PassFailJFrame.forceFail();
Provide the reason:
Suggestion:
PassFailJFrame.forceFail("stringFlavor is not supported by Transferable");
test/jdk/java/awt/dnd/RejectDragTest.java line 146:
> 144: endPoint = new Point(startPoint);
> 145: startPoint.translate(50, 50);
> 146: endPoint.translate(150, 150);
Let's make it correct, so that the object fields aren't modified after assigning the reference to the volatile field:
Suggestion:
Point start = frame.getLocationOnScreen();
start.translate(50, 50);
startPoint = start;
Point end = new Point(start);
end.translate(150, 150);
endPoint = end;
-------------
PR Review: https://git.openjdk.org/jdk/pull/21213#pullrequestreview-2343256950
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784734922
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784741660
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784747003
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784757794
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784761566
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784783294
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784791243
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784818439
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784823130
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784821200
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784823772
PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1784809675
More information about the client-libs-dev
mailing list