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.mes...@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
