On Tue, 29 Nov 2022 01:07:11 GMT, Damon Nguyen <[email protected]> wrote:
>> java/awt/PopupMenu/PopupMenuLocation.java seems to be unstable in MacOS
>> machines, especially in MacOSX 12 & 13 machines. It seems to be a testbug as
>> adding some stability improvements fixes the issue. It intermittently fails
>> in CI causing some noise. This test was already problem listed in windows
>> due to an umbrella bug JDK-8238720. This fix doesn't cover the Windows
>> issue, so the problem listing in windows will remain same.
>>
>> Fix:
>> Some stability improvements have been done and the test has been run 100
>> times per platform in mach5 and got full PASS.
>> Also updated the screen capture code, made frame undecorated, added some
>> prints for better debugging.
>
> test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 75:
>
>> 73: while (point.y < currentScreenBounds.y +
>> currentScreenBounds.height - insets.bottom - SIZE) {
>> 74: while (point.x <
>> 75: currentScreenBounds.x +
>> currentScreenBounds.width - insets.right - SIZE) {
>
> This might not be the most elegant way to abide by the rule to create a new
> line for long lines. I believe the line limit is 80 characters, which this
> still violates. And since it looks like you're trying to fix that here, might
> as well do it correctly.
I agree, the first `while` condition doesn't fit 80-column limit anyway, so the
second may violate it too, and I think the readability only improves.
Especially if you use `ScreenBounds` instead of `currentScreenBounds`.
Please, correct the positions of the closing braces: all of them are moved four
spaces too far from their level, including the one which closes `main` method
implementation.
> test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 114:
>
>> 112: private void show(MouseEvent e) {
>> 113: if (e.isPopupTrigger()) {
>> 114: System.out.println("Going to show popup "+pm+"
>> on "+frame);
>
> There seems to be a lack of consistency with spacing throughout the test.
> Spacing out the "+" from the quotations and the vars makes it cleaner.
Right; add the spaces around binary operators.
However, I'd probably drop this print as well as the one from the
`ActionListener`. Yet I admit these may help identifying what went wrong if the
test fails.
> test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 133:
>
>> 131: robot.mousePress(InputEvent.BUTTON3_DOWN_MASK);
>> 132: robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK);
>> 133: robot.waitForIdle();
>
> When testing locally on my macOS machine, this waitForIdle seems to be the
> culprit for why this test takes much longer per iteration. Maybe worth
> looking into why?
Yes, this `robot.waitForIdle` makes the test unbearably slow. Worth submitting
a separate bug with the reproducer. At this point, the pop is shown on the
screen and its first item is highlighted.
I replaced it with
robot.delay(200);
> test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 134:
>
>> 132: robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK);
>> 133: robot.waitForIdle();
>> 134: y = y+50;
>
> Another instance of the spacing issue. There seems to be more in this test,
> so it's probably best to fix them all.
This line has wrong indentation. I suggest using a compact syntax:
Suggestion:
y += 50;
or
Suggestion:
y += OFFSET;
if you create the `OFFSET` constant as I suggested above.
> test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 151:
>
>> 149: try {
>> 150:
>> ImageIO.write(robot.createScreenCapture(currentScreenBounds), "png",
>> 151: new File("screen1.png"));
>
> Is screen1.png the best name for the image? May be misleading if there are
> multiple screens, especially since this test iterates multiple locations.
It is definitely not. Either keep track of the current screen or use a generic
`screen.png` file name.
I would also suggest, wrapping all the parameters to `write`:
ImageIO.write(robot.createScreenCapture(screenBounds),
"png",
new File("screen1.png"));
This way the `"png"` doesn't get “lost”.
-------------
PR: https://git.openjdk.org/jdk/pull/10655
