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

Erik Helin erik.helin at oracle.com
Fri May 13 07:36:07 UTC 2016


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?

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?

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


More information about the hotspot-dev mailing list