jmx-dev RFR: 8004926 sun/management/jmxremote/bootstrap/CustomLauncherTest.sh oftenly times out

Thu Sep 12 09:05:19 PDT 2013

On 09/12/2013 05:53 PM, Dmitry Samersoff wrote:
> Jaroslav,
> 
> I'm not sure that replacing 50 lines shell script with 250 lines Java
> program we should keep it close to original. ;)

Well, I've deliberately dropped the intermittent failure generator ;)
And it's 150 against 250 ...

What I meant was that reviewers were often complaining when the old and
the new version had nothing in common at all. So, when I saw something like

 105 if [ ! -f "${JVMLIB}" ]; then
 106     JVMLIB="${TESTJAVA}/jre/lib/${LIBARCH}/server/libjvm.so"
 107     if [ ! -f "${JVMLIB}" ]; then
 108         JVMLIB="${TESTJAVA}/lib/${LIBARCH}/client/libjvm.so"
 109         if [ ! -f "${JVMLIB}" ]; then
 110             JVMLIB="${TESTJAVA}/lib/${LIBARCH}/serevr/libjvm.so"
 111             if [ ! -f "${JVMLIB}" ]; then
 112                 echo "Unable to locate libjvm.so in ${TESTJAVA}"
 113                 exit 1
 114             fi
 115         fi
 116     fi
 117 fi

I just directly copied it to java. It's nasty but at least one can see
where it came from and that it used to work before ...

But this is just nitpicking. I completely agree that it can (and should)
be made more readable. Always leave the code in the better state than
before the fix, I say.

-JB-

> 
> -Dmitry
> 
> 
> On 2013-09-12 19:31, Jaroslav Bachorik wrote:
>> On 09/12/2013 05:13 PM, Dmitry Samersoff wrote:
>>> Jaroslav,
>>>
>>> CustomLauncherTest.java:
>>>
>>> 102: this check could be moved to switch at ll. 108
>>> otherwise test fails on "sunos" and "linux" because PLATFORM remains unset.
>>
>> Good idea. Thanks.
>>
>>>
>>> 129: I would prefer don't have pattern like this one ever in shell
>>> script. Could you prepare a list of VM's to check and just loop over it?
>>> It makes test better readable. Also I think nowdays we can always use
>>> server VM.
>>
>> I tried to mirror the original shell test as closely as possible. It
>> would be nice if we could rely on the "server" vm only. Definitely more
>> readable.
>>
>> -JB-
>>
>>>
>>> -Dmitry
>>>
>>>
>>> On 2013-09-12 18:17, Jaroslav Bachorik wrote:
>>>> On 09/12/2013 10:22 AM, Jaroslav Bachorik wrote:
>>>>> On 09/12/2013 10:12 AM, Chris Hegarty wrote:
>>>>>> On 09/12/2013 04:45 AM, David Holmes wrote:
>>>>>>> Hi Jaroslav,
>>>>>>>
>>>>>>> You need a copyright notice in the new file.
>>>>>>>
>>>>>>> As written this test can only run on a full JDK - so please add it to
>>>>>>> the :needs_jdk group in TEST.groups. (Does jcmd really needs to come
>>>>>>> from the test-jdk? And use the VMOPTS passed to the test?)
>>>>>>>
>>>>>>> Is there a reason this test can't run on OSX? I know it would need
>>>>>>> further modification but was wondering if there is something inherent in
>>>>>>> the test that makes it inapplicable to OSX.
>>>>>>>
>>>>>>> I think the test would be a lot simpler if the jdk tests had the hotspot
>>>>>>> test library's process tools available. :(
>>>>>>
>>>>>> We have some, is there an obvious gap?
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/e407df8093dc/test/lib/testlibrary/jdk/testlibrary/
>>>>>
>>>>> Hm, thanks for the info. I should have used this library instead.
>>>>>
>>>>> Please, stand by for the updated webrev.
>>>>
>>>> I was able to get rid off the JCMD. Using the testlibrary the target
>>>> application can recognize its own PID and print it to its stdout. The
>>>> main application then just reads the stdout to parse the PID. No need
>>>> for JCMD any more.
>>>>
>>>> I could not find a way to remove the dependency on "test.jdk" system
>>>> property. According to the jtreg web documentation
>>>> (http://openjdk.java.net/jtreg/vmoptions.html#cmdLineOpts) a "test.java"
>>>> system property should be available but in fact is not. But it seems
>>>> that the testlibrary uses "test.jdk" system property too.
>>>>
>>>> The test does not run on OSX because nobody built the launcher binary :)
>>>> I think it is a kind of DIY so I took the liberty of adding a
>>>> linux-amd64 launcher while working on the test.
>>>>
>>>> While working with the test library I realized I was missing a crucial
>>>> feature (at least for my purposes) - waiting for a certain message to
>>>> appear in the stdout/stderr of the launched process. Very often I need
>>>> to wait for the target process to get to certain point before the test
>>>> can be allowed to continue - and the point is indicated by a message in
>>>> stdout/stderr. Currently all the proc tools are designed to work in
>>>> "batch" mode - the whole stdout/stderr is captured in strings and
>>>> analyzed after the target process died - and are not suitable for this
>>>> kind of usage.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8004926/webrev.01
>>>>
>>>>>
>>>>> -JB-
>>>>>
>>>>>>
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>> On 12/09/2013 1:39 AM, Jaroslav Bachorik wrote:
>>>>>>>> Please, review the patch for an intermittently failing test.
>>>>>>>>
>>>>>>>> The test is a shell test, using files for the interprocess
>>>>>>>> synchronization. This leads to intermittent failures.
>>>>>>>>
>>>>>>>> In order to fix this the test is rewritten in Java - the original
>>>>>>>> functionality and outputs should be 100% preserved. The patch is
>>>>>>>> unfortunately a bit difficult to follow since there is no similarity
>>>>>>>> between the *.sh and *.java file so one needs to go through the new
>>>>>>>> source in whole.
>>>>>>>>
>>>>>>>> The changes in "launcher" files are all about adding permissions to
>>>>>>>> execute (0755) and as such the webrev shows no differences.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Issue  : JDK-8004926
>>>>>>>> Webrev : http://cr.openjdk.java.net/~jbachorik/8004926/webrev.00
>>>>>>>>
>>>>>>>> -JB-
>>>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
> 