<AWT Dev> RFR: 8267161 : Write automated test case for JDK-4479161 [v4]

[lawrence.andrews]

On Wed, 18 Aug 2021 21:56:51 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> lawrence.andrews has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed review comments
>
> test/jdk/java/awt/im/4959409/bug4959409.java line 24:
> 
>> 22:  */
>> 23: 
>> 24: /**
> 
> I suggest to remove the second `*` to avoid the warning from the IDE about the dangling Javadoc comment.

removed

> test/jdk/java/awt/im/4959409/bug4959409.java line 147:
> 
>> 145: 
>> 146:         if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) {
>> 147:             throw new RuntimeException("Did not received KEY PRESS for Shift + 1 , test failed");
> 
> Suggestion:
> 
>             throw new RuntimeException("Did not receive KEY PRESS for Shift + 1 , test failed");
> 
> And the same for “KEY PRESS”.

Done

> test/jdk/java/awt/im/4959409/bug4959409.java line 170:
> 
>> 168:     }
>> 169: 
>> 170:     public static boolean isTopLevelVisible(javax.swing.JFrame jFrame, CountDownLatch topLevelVisibleLatch) throws Exception {
> 
> The logic in this method polls `jFrame.isVisible` instead of waiting for it to become visible. Nothing wrong with this approach yet the last call `topLevelVisibleLatch.await(TIMEOUT, TimeUnit.SECONDS)` doesn't make sense in this case as the latch can't reach 0 if it's not zero.
> 
> So, if something goes wrong, the test still waits for `TIMEOUT` (30) seconds for the condition that can never be satisfied. I suggest returning `topLevelVisibleLatch.getCount() == 0` and save 30 seconds.
> 
> Probably it's enough to check whether the text field is visible. The text field can become visible only if frame is visible, both likely become visible at the same moment anyway.

Removed this method

> test/jdk/java/awt/im/4959409/bug4959409.java line 216:
> 
>> 214:             if (frame != null) {
>> 215:                 SwingUtilities.invokeAndWait(frame::dispose);
>> 216:             }
> 
> This has inconsistency where `frame != null` is run on main thread without synchronisation. The null check should also be run on the EDT as I originally suggested.

Done

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

PR: https://git.openjdk.java.net/jdk/pull/5058