On Mon, 2 Dec 2024 17:42:21 GMT, Damon Nguyen <[email protected]> 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
