Hi Pavel, sure, Should I use the http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171806 that I created as CR for the test or the same CR 6800513?
Cheers, Mario 2012/5/30 Pavel Porvatov <pavel.porva...@oracle.com>: > Hi Mario, > > I marked CR 6800513 as fixed. Could you please push the test (with me as a > reviewer)? > > Thanks, Pavel > >> I created one: >> >> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171806 >> >> Should I wait that it becomes public or can i just push the "fix" >> right away (same reviewer and repository of course)? >> >> Cheers, >> Mario >> >> 2012/5/25 Alexander Scherbatiy<alexandr.scherba...@oracle.com>: >>> >>> On 5/25/2012 4:24 PM, Mario Torre wrote: >>>> >>>> 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? >>> >>> >>> I guess that it is not possible to make one more commit with the same >>> bug >>> id. >>> >>> Just create an issue that the test should be added to the repository. >>> >>> Thanks, >>> Alexandr. >>> >>> >>>> 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/
