Jarsolav, Looks good for me, comments below is just a nits - so fill free to ignore it.
1. As FS.getPath(TEST_JDK, "jre", "lib", LIBARCH) is the only value for findLibjvm parameter, It's better to create an overload function findLibjvm(). 2. it's better to check for File.isFile() - readable (e.g. device) is not always what you whant here. 3. It's good to try ARCH/libjvm.so, ARCH/server/libjvm.so, ARCH/client/libjvm.so in order for the possible platforms with the only vm -Dmitry On 2013-10-07 18:14, 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- >>>>>>>>> >>>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
