Hi Pavel, I uploaded a new patch here:
http://cr.openjdk.java.net/~neugens/6800513/webrev.02/ I don't really understand why one should call internal private api (realSync) when a public API is there (Toolkit.sync), that *should* do the same (even if it obviously doesn't). Anyway, I hope this version is good enough for you to go in. Please, let me know what you think, Mario 2012/5/4 Pavel Porvatov <pavel.porva...@oracle.com>: > 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 > -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF IcedRobot: www.icedrobot.org Proud GNU Classpath developer: http://www.classpath.org/ Read About us at: http://planet.classpath.org OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/
