RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified

[Leonid Mesnik]

Added Ioi, who also proposed new version of startAppVmOpts.

Please find new webrev: 
http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/

Renamed startAppVmOpts/runAppVmOpts to 
"startAppExactJvmOpts/runAppExactJvmOpts" is used. It should make very 
clear that this method doesn't use any of test.java.opts, test.vm.opts.

Also, I fixed test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java 
metnioned by Igor, and removed null pointer check as Ioi suggested in 
startApp method.

+ public static void startApp(LingeredApp theApp, String... 
additionalJvmOpts) throws IOException {
+ startAppExactJvmOpts(theApp, Utils.appendTestJavaOpts(additionalJvmOpts));
+ }

Leonid

On 3/25/20 10:14 AM, Stefan Karlsson wrote:
> On 2020-03-25 17:40, Igor Ignatyev wrote:
>> Hi Leonid,
>>
>> I have briefly looked at the patch, a few comments so far:
>>
>> test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java:
>>   - at L#114, could you please call static method using class name 
>> (as the opposite of using instance)? or was it meant to be 
>> theApp.runAppVmOpts(vmArgs) ?
>>
>> test/lib/jdk/test/lib/apps/LingeredApp.java:
>> - it seems that code indent of startApp(LingeredApp, String[]) isn't 
>> correct
>> - I don't like startAppVmOpts name, but unfortunately don't have a 
>> better suggestion (yet)
>
> I was going to say the same. Jtreg has the concept of "java options" 
> and "vm options". We have had a fair share of bugs and wasted time 
> when tests have been using the "vm options" part (VM_OPTIONS, 
> test.vm.options, etc), and we've been moving away from using that way 
> to pass options. I recently cleaned up some of this with:
>
> 8237111: LingeredApp should be started with getTestJavaOpts
>
> Because of this, I would prefer if we used a name that doesn't include 
> "VmOpts", because it's too alike the other concept. Some suggestions:
>  startAppJavaOptions
>  startAppUsingJavaOptions
>  startAppWithJavaOptions
>  startAppExactJavaOptions
>  startAppJvmOptions
>
> Thanks,
> StefanK
>
>> Thanks,
>> -- Igor
>>
>>> On Mar 25, 2020, at 8:55 AM, Leonid Mesnik 
>>> <leonid.mesnik at oracle.com> wrote:
>>>
>>> Hi
>>>
>>> Could you please review following fix which change LingeredApp to 
>>> prepend vm options to java/vm.test.opts when startApp is used and 
>>> provide startAppVmOpts to override options completely.
>>>
>>> The intention is to avoid issue like in this bug where test/jtreg 
>>> options were ignored by tests. Also I fixed some tests where 
>>> intention was to append vm options rather than to override them.
>>>
>>> webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8240698
>>>
>>> Leonid
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200325/96c4657c/attachment.htm>