Hi Serguei,
Thanks for the review. I considered a refactoring like that. However,
I'm going to expand this test when I push my fix to add support for the
clhsdb "sysprops" commands (one of the clhsdb commands that was lost
when we lost JS support). So I'll be executing that command also in this
test, and doing so using the existing ClhsdbLauncher support, which
means not directly using JDKToolLauncher. So it kind of seems odd to
have 4 ways of producing the properties list, but only two of them use
this shared code. However, when I add the clhsdb sysprops support, I'll
see if some refactoring makes sense. Maybe I could maintain the context
of each of the 3 tools that are run in separate class instances, and
possibly some of the implementation of a run() method could look like
when you envisioned. I think it's best to I see the code for all 3 ways
of generating the properties before doing that.
As for the check on the number of properties found, my concern was
having properties in one of two jinfo lists that are not in the output
of LingeredAppSysProps. So at first I was thinking I would need to use
each list as a primary list and have it compare against the other two,
but then I realized I could just use the count of the number of
properties found as a sanity check.
thanks,
Chris
On 4/7/20 12:00 AM, [email protected] wrote:
Hi Chris,
It looks good in general.
But a couple of comments.
http://cr.openjdk.java.net/~cjplummer/8242165/webrev.03/test/hotspot/jtreg/serviceability/sa/TestSysProps.java.html
I'm thinking if it'd make sense to do this kind of refactoring:
OutputAnalyzer runSACommand(String cmd, String argsStr, long pid)
throws Exception {
JDKToolLauncher launcher =
JDKToolLauncher.createUsingTestJDK(cmd);
for (String arg : argsStr.split(" ")) {
launcher.addToolArg(arg);
}
launcher.addToolArg(Long.toString(pid));
ProcessBuilder pb = SATestUtils.createProcessBuilder(launcher);
System.out.println("> " + ProcessTools.getCommandLine(pb));
Process process = pb.start();
OutputAnalyzer out = new OutputAnalyzer(process);
process.waitFor();
System.out.println(out.getStdout());
System.err.println(out.getStderr());
return out;
}
OutputAnalyzer jhsdbOut = runSACommand("jhsdb", "jinfo --sysprops
--pid", app.getPid());
OutputAnalyzer jinfoOut = runSACommand("jinfo", "-sysprops",
app.getPid());
jhsdbOut.shouldMatch("Debugger attached successfully.");
jinfoOut.shouldMatch("Java System Properties:");
Even though it does not seem to give a big advantage for this
particular test it creates a useful pattern that can be replicated
later if similar tests are needed in the future.
Also, I wonder if the code at lines 158-178 is really needed.
We should get a RuntimeException if any of the properties is not found
in the jhsdb or jinfo output.
So, the number of printed options should match.
But maybe I'm missing something.
Thanks,
Serguei
On 4/5/20 22:49, Chris Plummer wrote:
Hello,
Please help review the following:
https://bugs.openjdk.java.net/browse/JDK-8242165
http://cr.openjdk.java.net/~cjplummer/8242165/webrev.03
Please see the CR for an explanation of the bug and the fix. If you
need some help with the SA code, let me know and I'll provide some
details. It was pretty much all new to me, so I tried to put the
needed details in the CR.
For the test, I compare the list of properties dumped using 3
different methods to make sure the same set is dumped for each:
1. jhsdb jinfo --sysprops
2. jinfo -sysprops
3. Simple app (LingeredAppSysProps) that calls
System.getProperties().list(System.out)
The app output is considered the master list that is compared against
the others, and I also make sure each list has the same number of
properties.
thanks,
Chris
