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

[David Holmes]

On 9/10/2013 9:31 PM, Jaroslav Bachorik wrote:
> On 9.10.2013 12:23, David Holmes wrote:
>> Jaroslav,
>>
>> Thanks for the details description of changes - much appreciated.
>>
>> There is a lot to digest in there. :)
>
> Yep, it started as a simple fix :/
>
>>
>> It isn't obvious to me why these tests require a full JDK?
>
> IDK, LocalManagementTest.sh was listed as one requiring full JDK. Its
> requirements are the same as the ones of CustomLauncherTest.sh (now
> *.java) so it seemed logical to list it there too.

Ah! Now I see it - it uses tools.jar which implies 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.
>
> I'm afraid I can't be of much assistance here - I just took what was in
> the *.sh version and converted it to *.java.

Okay. I expect this will need revisiting at some point.

Thanks,
David
-----


> -JB-
>
>>
>> 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-
>>>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>