Ops, I just noticed that the test was not in the changeset, I applied the patch on a clean tree and apparently forgot to do hg add...
Should i push it with the same bug id? or do I need another id for that? Cheers, Mario 2012/5/25 Mario Torre <neugens.limasoftw...@gmail.com>: > Hi Pavel, > > Thanks, > > I've pushed to the awt gate. > > Cheers, > Mario > > 2012/5/24 Pavel Porvatov <pavel.porva...@oracle.com>: >> Hi Mario, >> >> >>> 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). >> >> Why do you think they should do the same? >> >>> >>> Anyway, I hope this version is good enough for you to go in. >> >> Now the test looks without functionality problems but there are some code >> style mistakes and unnecessary code. E.g. duplicate code in the main method, >> class field passing as method parameters (the getPopup method) etc. >> >> To avoid time spending I modified your test a little bit (see attach) and >> approve the fix. >> >> Regards, Pavel >> >>> 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/
