RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg

[Bengt Rutisson]

Looks good.

Bengt



On 1/18/13 1:43 PM, Christian Törnqvist wrote:
> Another updated webrev at http://cr.openjdk.java.net/~ctornqvi/webrev/8006413/webrev.02/ , formatting changes.
>
> Thanks,
> Christian
>
> -----Original Message-----
> From: Christian Törnqvist
> Sent: den 17 januari 2013 17:44
> To: hotspot-dev at openjdk.java.net
> Cc: Vitaly Davidovich; Yekaterina Kantserova
> Subject: RE: RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg
>
> Updated webrev can be found at http://cr.openjdk.java.net/~mgerdin/ctornqvi/8006413/webrev.01/ . Lots of small changes based on all the great feedback I got :)
>
> Thanks,
> Christian
>
> -----Original Message-----
> From: Christian Törnqvist
> Sent: den 17 januari 2013 15:19
> To: Vitaly Davidovich
> Cc: hotspot-dev at openjdk.java.net
> Subject: RE: RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg
>
> Hi Vitaly,
>
> First of all, thanks for all your feedback :)
>
>> - not entirely sure what getPlatformSpecificVMArgs is really meant for.  Maybe just have an is64bitJVM() instead? You can just return "new String[0]" though (I'm not a fan of extra >braces)
> This is for dealing with platform specific arguments to the java launcher (more importantly, not having the test worry about these things). Right now we only have the requirement to send '-d64' to any process launched as 64bit on Solaris, in the future we might have other requirements. That's the reason for giving this a more general name.
>
>> - there's no non-reflective way to get the pid?
> Not that I've found :(
>
>   
>
> I've taken a lot of your comments and made changes to the classes, I'll be publishing an updated webrev soon.
>
>   
>
> Thanks,
>
> Christian
>
>   
>
> From: Vitaly Davidovich [mailto:vitalyd at gmail.com]
> Sent: den 16 januari 2013 16:30
> To: Christian Törnqvist
> Cc: hotspot-dev at openjdk.java.net
> Subject: RE: RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg
>
>   
>
> Some of this is style so feel free to ignore:
>
> I'd mark all these classes final for now.  I'd also hide the default ctor for the util classes that only have static methods.
>
> JDKToolFinder
> - Check that the jdk.test sysprop is not empty as well.
>
> - Remove the throws clause from the method - don't think it adds any value.
>
> - RuntimeException seems more idiomatic instead of Exception
>
> - after creating the full path string, might be nice to verify that it exists and throw if not (things will not work later but you can provide a better error message here).  Perhaps even return a File or Path instance instead of string.
>
> OutputAnalyzer
> - any reason fields are default visible? Make them private and final.
>
> - should probably check that output is not null in the constructors
>
> - it's possible to chain the constructors here
>
> - same comment about throws clauses on methods + RuntimeException as above
>
> ProcessTools
> - getOutput swallows IOException and returns "".  Not sure this is best - I think the test should fail as you can't trust the results at this point.
>
> - StreamPumper extends Thread so I don't think you need the wrapper threads here.  Cleaner is to make StreamPumper implement Runnable though and use threads here.
>
> - interruption is caught and restored but no guarantee caller will observe it now. Again, don't think returning "" is right as results are indeterminate.  I'd let the interrupt bubble up.
>
> - there's no non-reflective way to get the pid?
>
> - not entirely sure what getPlatformSpecificVMArgs is really meant for.  Maybe just have an is64bitJVM() instead? You can just return "new String[0]" though (I'm not a fan of extra braces)
>
> -createJavaProcessBuilder() can probably just use List.addAll(Arrays.asList(...)) but Collections. addAll is OK too.
>
> StreamPumper
> - check ctor args for null reference.  The fields can be final as well.
>
> That's all for now :).
>
> Thanks
> Sent from my phone
>
> On Jan 16, 2013 9:42 AM, "Christian Törnqvist" <HYPERLINK "mailto:christian.tornqvist at oracle.com"christian.tornqvist at oracle.com> wrote:
>
> Hi Vitaly,
>
>   
>
> Setting them as daemon sounds like a good idea :)
>
>   
>
> I'd like your feedback on the other things as well, the quality of test code is important.
>
>   
>
> Thanks,
>
> Christian
>
>   
>
> From: Vitaly Davidovich [mailto:HYPERLINK "mailto:vitalyd at gmail.com" \nvitalyd at gmail.com]
> Sent: den 16 januari 2013 14:50
> To: Christian Törnqvist
> Cc: HYPERLINK "mailto:hotspot-dev at openjdk.java.net" \nhotspot-dev at openjdk.java.net
> Subject: Re: RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg
>
>   
>
> Hi Christian,
>
> Not sure how polished you want/need this code to be since it's just utility, but one thing that jumped out was the stream pumper threads for reading stdout/err should be daemonized just to play it safe.
>
> There are some other things that can be done, but not sure it's worth the effort for a testing utility.  Let me know though if you'd like to cover everything.
>
> Thanks
>
> Sent from my phone
>
> On Jan 16, 2013 7:36 AM, "Christian Törnqvist" <HYPERLINK "mailto:christian.tornqvist at oracle.com" \nchristian.tornqvist at oracle.com> wrote:
>
> Hi everyone,
>
>   
>
> This RFE adds a few utility classes to make it a bit easier to write multi-process tests in jtreg, webrev can be found at http://cr.openjdk.java.net/~brutisso/8006413/webrev.00/
>
>   
>
> Thanks,
>
> Christian