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/
