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

[Leonid Mesnik]

Igor, Stefan

Thank you for feedback, see my comments inline.

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) ?
No, it is a plain bug. I wanted to use non-static method first and 
forget to change to classname.
>>
>> 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

I prefer 'startAppExactJvmOptions' (and same runApp..) to be clear that 
this method doesn't use default test options and whole combination 
should be prepared by user.

And left startApp(String .. addtionaJVmOpts) for cases when additional 
options are prepend to standard set.

Let me know if what do you think about this.

Leonid

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