Hi Mario,
2012/4/21 Pavel Porvatov<pavel.porva...@oracle.com>:
Hi Pavel,
About the test:
1. Now is 2012 :)
Ops...
2. You must access to Swing components only from the EDT (see
clickOnComponent(final Component comp) and other methods)
Not sure if I understand correctly, all the access is done in the EDT
already, unless I became very blind!
The tests are run from the EDT, only exception is checkPopup, which
just read a value after the execution, and I think this should be
safe.
Yes, I missed the fact that the clickOnComponent method invoked on EDT.
That's because you used robot.delay(50) in the method. There is no sense
to use sleep methods on the EDT therad: you just freeze any event
handling....
b.
loop
final Map<String, Boolean> tests = new HashMap<>();
tests.put("javax.swing.PopupFactory$HeavyWeightPopup", false);
tests.put("javax.swing.PopupFactory$LightWeightPopup", true);
for (final String test : tests.keySet()) {
can be replaced by two simple invocations
Actually, this means duplicate more code or introduce another method,
not sure if this makes the code cleaner, but I can do it if you prefer
so.
c. NoSuchFieldException, SecurityException, IllegalArgumentException,
IllegalAccessException can be replaced by Exception
d.
robot.delay(50);
robot.mousePress(InputEvent.BUTTON1_MASK);
robot.delay(50);
Just use Robot#setAutoDelay
etc.
5. latch must be volitile. After test rewriting I think this variable can be
removed at all
Note that tests should be readable and simplest as far as possible
The reason why the test is so complex is that I wanted to throw the
exact exception and don't mix the reflection related stuff with the
real test exception, that also basically means I don't want to save
the exception and rethrow it later on (I've seen this in some other
tests), I rather prefer to make this obvious and not hidden, but of
course the code gets longer, and everything is complicated by the EDT
invocations.
In your case reflection exception is also test failing.
Also, I'm not particularly happy with the use of reflection to access
the filed and check the class name, since we're testing against an
implementation detail, but I don't know how else I should test that we
create an heavy weight window (which is really also just an
implementation detail that leaked through the code up to the user,
nobody should ever care about heavy weight and lightweight imho), so
if you have a smarter idea, I would be happy to change the code.
I'm also trying to avoid reflection in tests but I don't see another
solution here
I will try to refactor the code but I would like to not invest
significant time in that, I'll send you a revised patch later
(hopefully!)
Yes, and that's the reason to write first version of test without any
errors. The test shouldn't have errors, because if it fails (on some
platforms with very specific configuration) we have to fix it (therefore
we are trying to keep all tests as clear and short as possible)...
Your test is still have problems. E.g. setVisible invocation doesn't
guarantee that right after method Frame becomes visible (platforms
dependent behaviour). You can take a look at good test examples in
repository, e.g. here
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/dfa2ea47257d
Regards, Pavel
