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

Dmitriy Ermashov dmitriy.ermashov at oracle.com
Thu Apr 10 09:53:55 UTC 2014


Hi, Petr,

General comment:
Ugly parts of code (like 4, 5, 6, 7, 8) are just legacy of functional 
tests. We will be trying to get rid of them.
See next version of webrev soon.

On 04/09/2014 02:24 PM, Petr Pchelko 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..
I tried to rewrite tests on using throw new RuntimeException, and it 
seems to make more sense.
> 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.
Actually there is no need in result variable at all in scope on using 
"throw new Exception".
> 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
>>

-- 
Thanks,
Dima



More information about the awt-dev mailing list