Ping? Mario
2012/5/14 Mario Torre <neugens.limasoftw...@gmail.com>: > 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/ -- 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/
