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

[serguei.spitsyn at oracle.com]

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