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