RFR(M) 8242165: SA sysprops support fails to dump all system properties

[Chris Plummer]

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, serguei.spitsyn at oracle.com 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
>