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