RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v4]

[Alexey Ivanov]

On Wed, 4 Oct 2023 09:30:55 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Test was run without waiting for UI to be made visible leading to IllegalComponentStateException.
>> Used robot.delay consistent with other headful tests to made the test wait after UI is created and shown.
>> 
>> Test passed in several iterations in all platforms. Link in JBS
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   jcheck

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/JMenuItem/4654927/bug4654927.java line 1:

> 1: /*

I looked at the test code more thoroughly. I guess it can be improved.

Since GitHub doesn't allow adding comments to lines which weren't modified, I'll add my comment here.

**The first case** is straightforward:

1. Click the menu on the menu bar to open it,
2. Click the greyed out menu item.

The test saves the location of menu into `point`, then replaces it with location of the menu item. Yet it does request the same positions later in the test where it saves them into `menuLocation` and `itemLocation` correspondingly. Why not do it right away? The positions shouldn't change. Anyway the test relies on this fact because it saves the positions before the menu gets hidden.

**The second case** tests mouse drag. It simulates clicking the menu to open its popup and then dragging the mouse to the menu item and then releasing the mouse button.

Here it's more confusing… If you save the values in the first case above, you can clean up the code.

Then closing the menu could be done *before* entering the block of code for dragging mouse. In my opinion, it will make the test code clearer.

test/jdk/javax/swing/JMenuItem/4654927/bug4654927.java line 92:

> 90:             point = Util.getCenterPoint(menu);
> 91:             robot.mouseMove(point.x, point.y);
> 92:             robot.delay(250);

This delay may help. I saw that moving mouse takes a bit of time on macOS, the following code heavily depends on the mouse being in the expected position. You may assert that mouse cursor is at the expected position.

On the other hand, mouse press and further movements shouldn't precede the first mouse move event. If it happens, it could be a bug in macOS. Thus I can't see how this can lead to the test failure.

Which case does the test fail by the way? Is it the first or the second?

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

PR Review: https://git.openjdk.org/jdk/pull/15677#pullrequestreview-1657258616
PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1345609705
PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1345616470