RFR(S) : 8156681 : Add jtreg wrapper for hotspot gtest tests

[Igor Ignatyev]

Hey Erik,

I've updated the fix to be more readable.

webrev: http://cr.openjdk.java.net/~iignatyev/8156681/webrev.02/
diff b/w webrev.02 and webrev.01 : http://cr.openjdk.java.net/~iignatyev/8156681-diif/webrev.01/

— Igor

> On May 13, 2016, at 10:36 AM, Erik Helin <erik.helin at oracle.com> wrote:
> 
> On 2016-05-12, Igor Ignatyev wrote:
>> Jesper,
>> 
>> 1st of all, thank you for review.
>> 
>> unfortunately, testing in our infra reveled some problems w/ the wrapper:
>> - gtest test library can be located right in ‘test.nativepath’ or in ‘test.nativepath’/../../gtest
>> - the wrapper pass all flags to gtestLauncher including flags which are not JVM flags, gtestLauncher pass all these args to JNI_CreateJavaVM, so we fail w/ 'Unrecognized option'
>> 
>> I have updated the webrev : http://cr.openjdk.java.net/~iignatyev/8156681/webrev.01/
> 
> I don't understand the first path. Is System.getProperty("test.nativepath") either
> "<test_image>/hotspot/jtreg/native" or "<test_image>/hotspot/gtest"? Or
> is it something else?  What do you want the final path to be?
the final path should be <test_image>/hotspot/gtest/<vm_variant>
> 
> 
> Is the indentation a bit off here?
>  62                 Stream.of(
>  63                         path.toString(),
>  64                         "-jdk",
>  65                         System.getProperty("test.jdk")),
>  66                 Arrays.stream(Utils.getTestJavaOpts())
>  67                       .filter(s -> s.startsWith("-X") || s.startsWith("-D")))
> 
> since you always want the first three arguments, is the following a bit easier to understand?
I split that into 3 statements.
> 
>  List<String> options = Arrays.stream(Utils.getTestJavaOpts())
>                               .filter(s -> s.startsWith("-X") || s.startsWith("-D"))
>                               .collect(Collectors.toList());
>  List<String> cmd = Arrays.asList(path.toString(), "-jdk", System.getProperty("test.jdk"));
>  cmd.addAll(options);
> 
> The following
>  70         OutputAnalyzer oa = ProcessTools.executeProcess(builder);
>  71         oa.shouldHaveExitValue(0);
> 
> could also be shortened to one line if you prefer that:
> 
>  ProcessTools.executeProcess(builder).shouldHaveExitValue(0);
> 
> Thanks,
> Erik
> 
>> 
>> could you please review the updated version?
>> 
>> Thanks,
>> — Igor 
>> 
>>> On May 10, 2016, at 8:32 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>> 
>>> Looks good!
>>> /Jesper
>>> 
>>> Den 10/5/16 kl. 18:31, skrev Igor Ignatyev:
>>>> http://cr.openjdk.java.net/~iignatyev/8156681/webrev.00/
>>>>> 71 lines changed: 71 ins; 0 del; 0 mod;
>>>> 
>>>> Hi all,
>>>> 
>>>> could you please integrate this small fix which adds a jtreg wrapper to run gtest tests?
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8156681
>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8156681/webrev.00/
>>>> 
>>>> Thanks,
>>>> — Igor
>>>> 
>>