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

[Alexey Ivanov]

On Tue, 17 Aug 2021 15:50:56 GMT, lawrence.andrews <github.com+87324768+lawrence-andrew at openjdk.org> wrote:

>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent. 
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before starting the user action via Robot.
>> 
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed review comments

Changes requested by aivanov (Reviewer).

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.

test/jdk/java/awt/im/4959409/bug4959409.java line 65:

> 63:         final CountDownLatch keyPressedEventLatch = new CountDownLatch(1);
> 64:         final Point points[] = new Point[1];
> 65:         final Rectangle rect[] = new Rectangle[1];

Suggestion:

        final Point[] points = new Point[1];
        final Rectangle[] rect = new Rectangle[1];


The IDE issues a warning about C-style array declaration.

test/jdk/java/awt/im/4959409/bug4959409.java line 103:

> 101:                     } else {
> 102:                         jLabel.setText("Did not received KEY PRESS for Shift+1");
> 103:                         System.out.println("Did not received KEY PRESS for Shift+1");

Suggestion:

                        jLabel.setText("Did not receive KEY PRESS for Shift+1");
                        System.out.println("Did not receive KEY PRESS for Shift+1");

Probably it makes sense to spell “KEYPRESS”, without the space, for consistency.

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”.

test/jdk/java/awt/im/4959409/bug4959409.java line 162:

> 160:     }
> 161: 
> 162:     public static void checkSwingTopLevelVisible(javax.swing.JFrame jFrame, CountDownLatch topLevelVisibleLatch) throws InterruptedException, InvocationTargetException {

Is there a reason why you use the fully qualified `javax.swing.JFrame` when it's imported?

I suggest wrapping `throws` declaration to next line; this line is longer than 100.

Both comments apply to the method below.

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.

test/jdk/java/awt/im/4959409/bug4959409.java line 177:

> 175:                 TimeUnit.SECONDS.sleep(1);
> 176:                 checkSwingTopLevelVisible(jFrame, topLevelVisibleLatch);
> 177:                 if (topLevelVisibleLatch.getCount() == 0) {

Maybe this condition could be in the while loop?

while (count <= 5 && topLevelVisibleLatch.getCount() != 0)

Then there's no need for explicit premature break from the loop.

test/jdk/java/awt/im/4959409/bug4959409.java line 186:

> 184:     }
> 185: 
> 186:     public static void checkJComponentVisible(javax.swing.JComponent jComponent, CountDownLatch componentVisibleLatch) throws InterruptedException, InvocationTargetException {

You're using fully qualified `javax.swing.JComponent` here: `JComponent` isn't imported. Why not import it?

Please also wrap `throws` declaration to the next line.

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.

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

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