RFR: 8288415: java/awt/PopupMenu/PopupMenuLocation.java is unstable in MacOS machines

[Alexey Ivanov]

On Tue, 29 Nov 2022 01:07:11 GMT, Damon Nguyen <dnguyen at openjdk.org> 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