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

[Petr Pchelko]

Hello, Dmitry.

> 2. ../../../../lib/testlibrary could be replaces to just /lib/testlibrary.
Actually this is incorrect, sorry.

With best regards. Petr.

On 09.04.2014, at 14:24, Petr Pchelko <petr.pchelko at oracle.com> wrote:

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