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

[Vitaly Davidovich]

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" <
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:vitalyd at gmail.com]
> *Sent:* den 16 januari 2013 14:50
> *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****
>
> ** **
>
> 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" <
> christian.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****
>