Hi Stuart, On St 8. květen 2013, 01:50:22 CEST, Stuart Marks wrote: > Hi Jaroslav, > > Great to see this shell test get rewritten! > > Looks like you're avoiding multiple JVM processes as well, by loading > the different versions of the classes into different classloaders. It > looks like a bit of trouble, but probably less than the amount of > trouble caused by the shell script. > > I have a couple minor points. > > The timeout value of one second seems quite low. Under normal > operation, spawning a couple threads and should proceed very quickly. > However, our testing environment is quite hostile, and things that > seem like they ought to proceed quickly often take considerably longer > than one might think. Since you're counting notifications, and in > normal operation they all come in, we don't wait for the actual > timeout unless there's a failure. So it might make sense to raise the > timeout to 10 or perhaps 30 seconds.
I've adjusted the timeout for 30 seconds. Hopefully, it will be enough. > > On the other hand, I have a question about whether counting the number > of notifications is correct. Would it be possible for there to be a > bug where an extra notification is sent? If so, this might mean that > the test would exit prematurely, indicating success? It would be a severe error in the notification system implementation. However, I've added a check for duplicated notifications (each notification carries its sequence number) which makes the test fail in case of duplication. But I don't expect it to happen. The updated webrev is at http://cr.openjdk.java.net/~jbachorik/8005472/webrev.09 -JB- > > s'marks > > On 5/6/13 2:04 AM, Jaroslav Bachorik wrote: >> On Pá 3. květen 2013, 16:16:53 CEST, Daniel Fuchs wrote: >>> Hi Jaroslav, >>> >>> In Client.java - you could consider replacing the AtomicLong >>> with a CountDownLatch. >>> >>> This would allow you to remove the various Thread.sleep() in the >>> code (in particular the one at the end). >>> >>> You could use CountDownLatch.await(long timeout, TimeUnit unit) to >>> avoid waiting for ever in case of bugs, and the advantage is that >>> the test would be able to exit as soon as the count down latch >>> reaches 0, without having to wait for an arbitrary timeout. >> >> Great, thanks for the pointer! I've changed the test to use the >> CountDownLatch. >> >> http://cr.openjdk.java.net/~jbachorik/8005472/webrev.08/ >> >> -JB- >> >>> >>> Very nice to see a shell test go away :-) >>> >>> -- daniel >>> >>> >>> On 5/3/13 3:41 PM, Jaroslav Bachorik wrote: >>>> Please re-review the updated webrev >>>> http://cr.openjdk.java.net/~jbachorik/8005472/webrev.06 >>>> >>>> I've replaced the shell script with the plain java test. The javac API >>>> is used to compile the the auxiliary classes as was recommended. This >>>> allowed to simplify the test. >>>> >>>> The test does not check for a certain string in the standard output >>>> anymore - it turns out that it is possible to count the number of all >>>> the received JMX notifications (even though some notifications can be >>>> lost, we receive a special notification with the number of the lost >>>> regular notifications). It is then possible to match the actual number >>>> of processed notifications (received + lost) against the expected >>>> number >>>> - different numbers mean that the notification processing thread had >>>> been interrupted unexpectedly. >>>> >>>> Thanks, >>>> >>>> -JB- >>>> >>>> On 8.2.2013 17:37, Chris Hegarty wrote: >>>>> >>>>>> Jon Gibbons suggested invoking the compiler API directly from java >>>>>> instead of writing a shell script. Doing this seems fairly simple, >>>>>> and I >>>>>> think it would be advantageous to keep things entirely in Java. I >>>>>> may >>>>>> attempt to rewrite the defaultSVID test using the compiler API. >>>>> >>>>> Here's a test that does just that. >>>>> >>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/2de8c6c2d652/test/sun/misc/JarIndex/metaInfFilenames/Basic.java >>>>> >>>>> >>>>> >>>>> >>>>> -Chris. >>>> >>> >> >>
