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

[Alexey Ivanov]

On Tue, 11 Oct 2022 14:17:42 GMT, Manukumar V S <mvs 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.

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 42:

> 40: import java.util.stream.IntStream;
> 41: import javax.imageio.ImageIO;
> 42: import javax.swing.SwingUtilities;

Remove the unused imports: `AtomicReference` and `SwingUtilities`.

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 59:

> 57:     private static Robot robot;
> 58:     private static Frame frame;
> 59:     private static Rectangle currentScreenBounds;

Suggestion:

    private static Rectangle screenBounds;

I think it's still clear enough yet shorter.

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 72:

> 70:             currentScreenBounds = gc.getBounds();
> 71:             Point point = new Point(currentScreenBounds.x, currentScreenBounds.y);
> 72:             Insets insets = Toolkit.getDefaultToolkit().getScreenInsets(gc);

Suggestion:

            Insets insets = Toolkit.getDefaultToolkit().getScreenInsets(gc);
            Point point = new Point(currentScreenBounds.x + insets.left, currentScreenBounds.y + insets.top);


The initial position of the frame should take the insets into account.

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 129:

> 127:         Point pt = frame.getLocationOnScreen();
> 128:         int x = pt.x + frame.getWidth() / 2;
> 129:         int y = pt.y + 50;

I suggest creating a constant `OFFSET` with the value of `50`. It would allow changing the position of the click easily.

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 143:

> 141:             throw new RuntimeException(
> 142:                     "Failed, Not received the PopupMenu ActionEvent yet on " +
> 143:                     "frame= "+frame+", isFocused = "+frame.isFocused());

Suggestion:

                    "Failed, didn't receive the PopupMenu ActionEvent on " +
                    "frame= " + frame + ", isFocused = " + frame.isFocused());

Grammar and spaces around the `+` operator.

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 153:

> 151:                           new File("screen1.png"));
> 152:         } catch (Exception exception) {
> 153:             exception.printStackTrace();

Suggestion:

        } catch (Exception e) {
            e.printStackTrace();

Why not use the standard name for the exception variable? Spelling the entire word doesn't add any value.

test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 155:

> 153:             exception.printStackTrace();
> 154:         }
> 155:         action = false;

Suggestion:


It's unneeded here and rather confusing.

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

PR: https://git.openjdk.org/jdk/pull/10655