<AWT Dev> Review request for JDK-8039279: Move first batch of functional tests to openjdk repository

[Petr Pchelko]

Hello, Dmitriy.

1. Could you please not use the applet pattern in automatic tests? 
We normally get rid of this in all the tests we touch/write, because it's useless and slows down the test execution.
So please rewrite SetBoundsTest to use main.

2. ../../../../lib/testlibrary could be replaces to just /lib/testlibrary.

3. I see that you are using printStackTrace + System.exit to fail the test. We normally just throw the RuntimeException
or state that main throws Exception, do not add catch blocks and let exceptions propagate out of main. jtreg would catch them,
report error nicely and fail the test.
I'm not sure which pattern is better, so it worths discussing..

4. ModifierRobotKey : System.getProperty("os.name").trim().substring(0, 3).toUpperCase().equals("SUN") you can replace this
to use the OSInfo class. It's way mode reliable.

5. ModifierRobotKey: Toolkit.getDefaultToolkit().getSystemEventQueue().invokeAndWait - Why no use SwingUtilities?

6. General comment: since this is going to 9 you could use lambdas wherever possible.

7. ModifierRobotTest: 131 canvas.addFocusListener(new FocusListener() - could be replaced with an Adapter.

8. Another general comment: the lib/testlibrary has an Asserts class - it could be used where possible. The

assertTrue(focusGained, "Canvas focus gained");

looks much cleaner than 

if (!focusGained) {
  System.err.println("FAIL: Canvas failed to gain focus!");
  System.exit(1);
}

Of coarse it could be used only if we switch to "throw exception and not catch" pattern instead of the "System.exit" pattern.

9. LockingKeyStateTest - I don't like the pattern with the "result" variable. First of all - why it's an int if it's really should be a boolean?
Secondly, why are we continuing the test execution after we've found an error? Who cares that all the rest of assertions are true or not,
the test is already failed anyway. I'd rather just throw an Exception in place and stop the execution. 

The most of my comments are a bit controversial, I just want to run the discussion of these points, because this is the first portion of tests
added and we are setting up the patterns now...

With best regards. Petr

On 08.04.2014, at 16:09, Dmitriy Ermashov <dmitriy.ermashov at oracle.com> wrote:

> Hi,
> Please, review the changeset for:
> https://bugs.openjdk.java.net/browse/JDK-8039279
> 
> Webrev is here:
> http://cr.openjdk.java.net/~yan/8039279/webrev/
> 
> This is the first batch of functional AWT tests, that are prepared for migration to OpenJDK repository.
> Testlist:
> AWT_ZoomFrame/Automated/Frame/SetBoundsTest
> AWT_Robot/Automated/ModifierRobotKey
> AWT_Toolkit/Manual/LockingKeyStateTest
> AWT_LayoutTest/Automated/GridLayout/Layout
> AWT_LayoutTest/Automated/GridLayout/ChangeGrids
> 
> Tests were switched on using jtreg, tested on the following platforms and seems stable:
> Windows 7 x64, Intel graphical card
> Ubuntu Linux 12.04 x64, Intel graphical card
> OS X 10.8.5 x64, NVIDIA GeForce 9400
> Ubuntu 10.04 ARMv7
> 
> -- 
> Thanks,
> Dima
>