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 <[email protected]> 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
