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

[Stefan Karlsson]

Thanks for changing the name. Sounds good to me. I leave the full review 
to others.

StefanK

On 2020-03-25 20:01, Leonid Mesnik wrote:
>
> 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
>>>>
>>