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

[David Holmes]

Jaroslav,

Thanks for the details description of changes - much appreciated.

There is a lot to digest in there. :)

It isn't obvious to me why these tests require a full JDK?

I don't quite follow the libjvm lookup logic - I would expect that you 
would always want to test the libjvm that is currently running - though 
it is hard to determine that.

Thanks,
David

On 8/10/2013 9:33 PM, Jaroslav Bachorik wrote:
> On 8.10.2013 05:42, David Holmes wrote:
>> Jaroslav,
>>
>> Can you summarise the changes please? With the conversion to Java and
>> the infrastructure additions I can't tell what is actually fixing the
>> original timeout issue :)
>
> The timeout was most caused by using the same file for communication
> between java processes in more test cases. When those test cases were
> run in parallel the file got rewritten silently and some of the tests
> could end up trying to connect to incorrect port in the target
> application. I was able to reproduce the timeout by interleaving the
> test runs for CustomLauncherTest.sh and LocalManagementTest.sh and
> adding an artificial delay to CusteomLauncherTest.sh to allow
> LocalManagementTest.sh to change the port in the file.
>
> While it could be fixed by using a different file for each test case I
> took the liberty of converting the shell tests to java tests. This
> allows me to remove the communication file and, in the end, make the
> tests more robust.
>
> CustomLauncherTest.java and LocalManagementTest.java are the tests
> converted from shell to java. I decided to convert
> LocalManagementTest.sh as well because it has the same problems as the
> CustomLauncherTest.sh.
>
> The changes in the testlibrary are about introducing new methods
> allowing the tests easily start a process and wait for a certain text
> appearing in its stdout/stderr. Using these methods the caller can wait
> till the callee is fully initialized and eg. ready to accept connections.
>
> The changes in launchers make the launchers actually executable + I am
> adding a linux-amd64 launcher (I needed that one to work on the changes
> locally and thought it might be nice to have one more platform covered
> by the test).
>
> I've update the webrev to include changes to LocalManagementTest and
> TEST.groups (both of those tests require JDK) -
> http://cr.openjdk.java.net/~jbachorik/8004926/webrev.05
>
> -JB-
>
>>
>> Thanks,
>> David
>>
>> On 8/10/2013 12:14 AM, Jaroslav Bachorik wrote:
>>> On 19.9.2013 16:33, Jaroslav Bachorik wrote:
>>>> The updated webrev:
>>>> http://cr.openjdk.java.net/~jbachorik/8004926/webrev.03
>>>>
>>>> I've moved some of the functionality to the testlibrary.
>>>>
>>>> -JB -
>>>>
>>>> On 12.9.2013 17: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-
>>>>>>>>>>>
>>>>>>
>>>>
>>>
>