Hi Chris,
I'm okay with your approaches.
Please, consider it reviewed.
Thanks,
Serguei
On 4/7/20 00:17, Chris Plummer wrote:
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
