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.
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?
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.
