RFR: 8340790: Open source several AWT Dialog tests - Batch 4 [v2]

[Tejesh R]

On Wed, 9 Oct 2024 16:53:27 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Updated review comments
>
> test/jdk/java/awt/Dialog/ChoiceModalDialogTest.java line 111:
> 
>> 109:             });
>> 110:             r.delay(500);
>> 111:             r.waitForIdle();
> 
> It is usually waitForIdle() followed by a delay().
> Here and at other locations.

Updated.

> test/jdk/java/awt/Dialog/ChoiceModalDialogTest.java line 120:
> 
>> 118:                 r.keyRelease(KeyEvent.VK_A);
>> 119: 
>> 120:             });
> 
> This test fails  with `keyOK = false` due to missing delay after KeyEvent.
> To make the test more stable, I think it might be better to convert  keyOK and mouseOK into CountDownLatch. If you use CountDownLatch with await of 2 seconds then there is no need to the extra delay.
> 
> 
>     private static volatile CountDownLatch keyOK = new CountDownLatch(1);
>     private static volatile CountDownLatch mouseOK = new CountDownLatch(1);
> 
> 
> Suggestion:
> 
>                 r.keyRelease(KeyEvent.VK_A);
>             });
>            r.waitForIdle(); 
>            r.delay(500);

I don't think `CountDownLatch` is required here. Anyhow I have added some delays after keypress too. Still didn't get failure in CI system. Please check with updated test once and revert.

> test/jdk/java/awt/Dialog/ChoiceModalDialogTest.java line 134:
> 
>> 132:         if (!mouseOK || !keyOK) {
>> 133:             throw new RuntimeException("Test FAILED");
>> 134:         }
> 
> This check can be moved before finally block and the test failure msg can be more descriptive.
> If you are converting into CountDownLatch then you need to change this check accordingly

Moved the block before finally.

> test/jdk/java/awt/Dialog/ChoiceModalDialogTest.java line 136:
> 
>> 134:         }
>> 135: 
>> 136:         System.out.println("Test Passed!");
> 
> `System.out.println("Test Passed!");` is redundant and can be removed.

It will useful for stdout print apart from jtreg output, hence retaining it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21404#discussion_r1794973007
PR Review Comment: https://git.openjdk.org/jdk/pull/21404#discussion_r1794972514
PR Review Comment: https://git.openjdk.org/jdk/pull/21404#discussion_r1794974883
PR Review Comment: https://git.openjdk.org/jdk/pull/21404#discussion_r1794973954