RFR: 8343736: Test java/awt/Focus/UnaccessibleChoice/AccessibleChoiceTest.java failed: Choice can't be controlled by keyboard [v3]

[Harshitha Onkar]

On Mon, 2 Dec 2024 17:42:21 GMT, Damon Nguyen <dnguyen at openjdk.org> wrote:

>> Test intermittently fails with a few different Exceptions. Initial report shows `Choice can't be controlled by keyboard` when failing. An additional report of an intermittent failure shows `Button does not have focus`.
>> 
>> Added some stability fixes. Additional delays, removed extraneous window, and added an additional focus check.
>> 
>> Debugged using additional screenshots during different failure points. Looks like sometimes the focus is still on the button. So, the delay has been added afterwards. Test passes on 22.04 Ubuntu machine with 100 repeats in CI. Also passed testing on all OS's with 50 repeats in CI.
>
> Damon Nguyen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Amend test

LGTM, apart from the minor inline comments.

test/jdk/java/awt/Focus/UnaccessibleChoice/AccessibleChoiceTest.java line 53:

> 51: public class AccessibleChoiceTest {
> 52:     //Declare things used in the test, like buttons and labels here
> 53:     Frame frame = new Frame("window owner");

Following comment can be removed.

`//Declare things used in the test, like buttons and labels here`

test/jdk/java/awt/Focus/UnaccessibleChoice/AccessibleChoiceTest.java line 56:

> 54:     static Choice choice;
> 55:     static Button button;
> 56:     static CountDownLatch go;

Suggestion:

    static volatile CountDownLatch go;

test/jdk/java/awt/Focus/UnaccessibleChoice/AccessibleChoiceTest.java line 112:

> 110:         });
> 111:         robot.mouseMove(loc.x + bWidth / 2, loc.y
> 112:                 + bHeight / 2);

Alignment

Suggestion:

        robot.mouseMove(loc.x + bWidth / 2,
                        loc.y + bHeight / 2);

test/jdk/java/awt/Focus/UnaccessibleChoice/AccessibleChoiceTest.java line 145:

> 143: 
> 144:         String osName = System.getProperty("os.name").toLowerCase();
> 145:         if (osName.startsWith("mac")) {

Suggestion: Platform.isOSX() looks cleaner. 
jtreg @library tag is required in case jdk.test.lib.Platform is used.

test/jdk/java/awt/Focus/UnaccessibleChoice/AccessibleChoiceTest.java line 159:

> 157:         if (!choice.getSelectedItem().equals(choice.getItem(1))) {
> 158:             // Print out os name to check if mac conditional is relevant
> 159:             System.err.println("Failed on os: " + osName);

Not strictly required to print osName in case of failure (since the details are available), okay to have it though.

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

Marked as reviewed by honkar (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22333#pullrequestreview-2474269859
PR Review Comment: https://git.openjdk.org/jdk/pull/22333#discussion_r1866839216
PR Review Comment: https://git.openjdk.org/jdk/pull/22333#discussion_r1866840155
PR Review Comment: https://git.openjdk.org/jdk/pull/22333#discussion_r1866853625
PR Review Comment: https://git.openjdk.org/jdk/pull/22333#discussion_r1866841396
PR Review Comment: https://git.openjdk.org/jdk/pull/22333#discussion_r1866849950