RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Jini George
Hello,

Requesting reviews for:

JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/

These are the new test cases for the following clhsdb commands:
1. inspect
2. scanoops
3. printas

These tests have been verified through the Mach5 and jprt systems.

Thanks,
Jini.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Jini George
Gentle Reminder.

Thank you.
Jini.

On 12/8/2017 12:33 PM, Jini George wrote:

> Hello,
>
> Requesting reviews for:
>
> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
> Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
>
> These are the new test cases for the following clhsdb commands:
> 1. inspect
> 2. scanoops
> 3. printas
>
> These tests have been verified through the Mach5 and jprt systems.
>
> Thanks,
> Jini.
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Sharath Ballal
In reply to this post by Jini George
Hi Jini,
Looks Good. Some nits:

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html

Since you are not passing any new VM options, the following lines

   28    import jdk.test.lib.Utils;
  47             List<String> vmArgs = new ArrayList<String>();
  48             vmArgs.addAll(Utils.getVmOptions());
  49
  50             theApp = new LingeredAppWithLock();
  51             LingeredApp.startApp(vmArgs, theApp);

Can be replaced by

theApp = new LingeredAppWithLock();
LingeredApp.startApp(null, theApp);

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbScanOops.java.html

41     public static void testWithGcType
If you are not planning on this method being called from elsewhere, you can make it private.

Thanks,
Sharath


-----Original Message-----
From: Jini George
Sent: Friday, December 08, 2017 12:33 PM
To: [hidden email]
Subject: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Hello,

Requesting reviews for:

JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/

These are the new test cases for the following clhsdb commands:
1. inspect
2. scanoops
3. printas

These tests have been verified through the Mach5 and jprt systems.

Thanks,
Jini.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Chris Plummer
In reply to this post by Jini George
Hi Jini,

On 12/7/17 11:03 PM, Jini George wrote:
> Hello,
>
> Requesting reviews for:
>
> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
> Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
>
> These are the new test cases for the following clhsdb commands:
> 1. inspect
tokensMap code can all move to inside the "if" block that follows it.
> 2. scanoops
When will "universe" not return any output?

> 3. printas
Ok.

thanks,

Chris
>
> These tests have been verified through the Mach5 and jprt systems.
>
> Thanks,
> Jini.
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Jini George
Thank you, Chris. Answers inline.

On 12/12/2017 4:43 AM, Chris Plummer wrote:
>> These are the new test cases for the following clhsdb commands:
>> 1. inspect
> tokensMap code can all move to inside the "if" block that follows it.
Done.

>> 2. scanoops
> When will "universe" not return any output?
universeOutput could be null if there are attach permission issues and
if we are skipping the test. I have added these comments there for clarity.

I have a new webrev at:

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/

Thanks,
Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Jini George
In reply to this post by Sharath Ballal
Thank you, Sharath. I have a modified webrev at:

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/

Could a Reviewer also please take a look at it ?

Thanks,
Jini.

On 12/11/2017 3:41 PM, Sharath Ballal wrote:

> Hi Jini,
> Looks Good. Some nits:
>
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html
>
> Since you are not passing any new VM options, the following lines
>
>     28    import jdk.test.lib.Utils;
>    47             List<String> vmArgs = new ArrayList<String>();
>    48             vmArgs.addAll(Utils.getVmOptions());
>    49
>    50             theApp = new LingeredAppWithLock();
>    51             LingeredApp.startApp(vmArgs, theApp);
>
> Can be replaced by
>
> theApp = new LingeredAppWithLock();
> LingeredApp.startApp(null, theApp);
>
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbScanOops.java.html
>
> 41     public static void testWithGcType
> If you are not planning on this method being called from elsewhere, you can make it private.
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: Jini George
> Sent: Friday, December 08, 2017 12:33 PM
> To: [hidden email]
> Subject: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands
>
> Hello,
>
> Requesting reviews for:
>
> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
> Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
>
> These are the new test cases for the following clhsdb commands:
> 1. inspect
> 2. scanoops
> 3. printas
>
> These tests have been verified through the Mach5 and jprt systems.
>
> Thanks,
> Jini.
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Sharath Ballal
Looks good Jini.


Thanks,
Sharath


-----Original Message-----
From: Jini George
Sent: Tuesday, December 12, 2017 4:09 PM
To: Sharath Ballal; [hidden email]
Subject: Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Thank you, Sharath. I have a modified webrev at:

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/

Could a Reviewer also please take a look at it ?

Thanks,
Jini.

On 12/11/2017 3:41 PM, Sharath Ballal wrote:

> Hi Jini,
> Looks Good. Some nits:
>
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html
>
> Since you are not passing any new VM options, the following lines
>
>     28    import jdk.test.lib.Utils;
>    47             List<String> vmArgs = new ArrayList<String>();
>    48             vmArgs.addAll(Utils.getVmOptions());
>    49
>    50             theApp = new LingeredAppWithLock();
>    51             LingeredApp.startApp(vmArgs, theApp);
>
> Can be replaced by
>
> theApp = new LingeredAppWithLock();
> LingeredApp.startApp(null, theApp);
>
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbScanOops.java.html
>
> 41     public static void testWithGcType
> If you are not planning on this method being called from elsewhere, you can make it private.
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: Jini George
> Sent: Friday, December 08, 2017 12:33 PM
> To: [hidden email]
> Subject: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands
>
> Hello,
>
> Requesting reviews for:
>
> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
> Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
>
> These are the new test cases for the following clhsdb commands:
> 1. inspect
> 2. scanoops
> 3. printas
>
> These tests have been verified through the Mach5 and jprt systems.
>
> Thanks,
> Jini.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

serguei.spitsyn@oracle.com
In reply to this post by Jini George
Hi Jini,

Some minor comments:
http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html

I'd suggest to unsplit some lines (check if same applies to other two
tests):

   56             String jstackOutput =
   57                 test.run(theApp.getPid(), cmds, null, null);
   ...

   78                             addressString =
   79                                 (token.replace("<", "")).replace(">", "");

   No need for brackets around the token.replace.

Need a space after 'for':

   70                 for(String key: tokensMap.keySet()) {



Q: In what condition the jstackOutput can be null?

      59             if (jstackOutput != null) {

    A suggestion is to use the same approach as in the ClhsdbScanOops test:

   60             if (universeOutput == null) {
   61                 // Output could be null due to attach permission issues
   62                 // and if we are skipping this.
   63                 LingeredApp.stopApp(theApp);
   64                 return;
   65             }


Otherwise, the fix looks good to me.

Thanks,
Serguei



On 12/12/17 02:38, Jini George wrote:

> Thank you, Sharath. I have a modified webrev at:
>
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/
>
> Could a Reviewer also please take a look at it ?
>
> Thanks,
> Jini.
>
> On 12/11/2017 3:41 PM, Sharath Ballal wrote:
>> Hi Jini,
>> Looks Good. Some nits:
>>
>> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html 
>>
>>
>> Since you are not passing any new VM options, the following lines
>>
>>     28         import jdk.test.lib.Utils;
>>    47             List<String> vmArgs = new ArrayList<String>();
>>    48             vmArgs.addAll(Utils.getVmOptions());
>>    49
>>    50             theApp = new LingeredAppWithLock();
>>    51             LingeredApp.startApp(vmArgs, theApp);
>>
>> Can be replaced by
>>
>> theApp = new LingeredAppWithLock();
>> LingeredApp.startApp(null, theApp);
>>
>> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbScanOops.java.html 
>>
>>
>> 41     public static void testWithGcType
>> If you are not planning on this method being called from elsewhere,
>> you can make it private.
>>
>> Thanks,
>> Sharath
>>
>>
>> -----Original Message-----
>> From: Jini George
>> Sent: Friday, December 08, 2017 12:33 PM
>> To: [hidden email]
>> Subject: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect',
>> 'scanoops' and 'printas' commands
>>
>> Hello,
>>
>> Requesting reviews for:
>>
>> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
>> Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
>>
>> These are the new test cases for the following clhsdb commands:
>> 1. inspect
>> 2. scanoops
>> 3. printas
>>
>> These tests have been verified through the Mach5 and jprt systems.
>>
>> Thanks,
>> Jini.
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Chris Plummer
In reply to this post by Jini George
Hi Jini,

On 12/12/17 2:38 AM, Jini George wrote:

> Thank you, Chris. Answers inline.
>
> On 12/12/2017 4:43 AM, Chris Plummer wrote:
>>> These are the new test cases for the following clhsdb commands:
>>> 1. inspect
>> tokensMap code can all move to inside the "if" block that follows it.
> Done.
>
>>> 2. scanoops
>> When will "universe" not return any output?
> universeOutput could be null if there are attach permission issues
Why would this ever happen in a testing environment?
> and if we are skipping the test.
I'm not sure what you meant by this. If we are running the test, how are
we skipping it?
> I have added these comments there for clarity.
My concern is that logic like this could be hiding some other issue.
It's possible that some other bug causes universeOutput to be null on
every run we do, yet the test will show as passed.

thanks,

Chris
>
> I have a new webrev at:
>
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/
>
> Thanks,
> Jini.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands

Jini George
In reply to this post by serguei.spitsyn@oracle.com
Thank you, Serguei. I have the modified webrev after addressing the
comments at:

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.02/

jstackOutput could be null if there are attach permission issues.

Thanks,
Jini.


On 12/13/2017 4:44 AM, [hidden email] wrote:

> Hi Jini,
>
> Some minor comments:
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html 
>
>
> I'd suggest to unsplit some lines (check if same applies to other two
> tests):
>
>    56             String jstackOutput =
>    57                 test.run(theApp.getPid(), cmds, null, null);
>    ...
>
>    78                             addressString =
>    79                                 (token.replace("<",
> "")).replace(">", "");
>
>    No need for brackets around the token.replace.
>
> Need a space after 'for':
>
>    70                 for(String key: tokensMap.keySet()) {
>
>
>
> Q: In what condition the jstackOutput can be null?
>
>       59             if (jstackOutput != null) {
>
>     A suggestion is to use the same approach as in the ClhsdbScanOops test:
>
>    60             if (universeOutput == null) {
>    61                 // Output could be null due to attach permission
> issues
>    62                 // and if we are skipping this.
>    63                 LingeredApp.stopApp(theApp);
>    64                 return;
>    65             }
>
>
> Otherwise, the fix looks good to me.
>
> Thanks,
> Serguei
>
>
>
> On 12/12/17 02:38, Jini George wrote:
>> Thank you, Sharath. I have a modified webrev at:
>>
>> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/
>>
>> Could a Reviewer also please take a look at it ?
>>
>> Thanks,
>> Jini.
>>
>> On 12/11/2017 3:41 PM, Sharath Ballal wrote:
>>> Hi Jini,
>>> Looks Good. Some nits:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html 
>>>
>>>
>>> Since you are not passing any new VM options, the following lines
>>>
>>>     28         import jdk.test.lib.Utils;
>>>    47             List<String> vmArgs = new ArrayList<String>();
>>>    48             vmArgs.addAll(Utils.getVmOptions());
>>>    49
>>>    50             theApp = new LingeredAppWithLock();
>>>    51             LingeredApp.startApp(vmArgs, theApp);
>>>
>>> Can be replaced by
>>>
>>> theApp = new LingeredAppWithLock();
>>> LingeredApp.startApp(null, theApp);
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbScanOops.java.html 
>>>
>>>
>>> 41     public static void testWithGcType
>>> If you are not planning on this method being called from elsewhere,
>>> you can make it private.
>>>
>>> Thanks,
>>> Sharath
>>>
>>>
>>> -----Original Message-----
>>> From: Jini George
>>> Sent: Friday, December 08, 2017 12:33 PM
>>> To: [hidden email]
>>> Subject: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect',
>>> 'scanoops' and 'printas' commands
>>>
>>> Hello,
>>>
>>> Requesting reviews for:
>>>
>>> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
>>>
>>> These are the new test cases for the following clhsdb commands:
>>> 1. inspect
>>> 2. scanoops
>>> 3. printas
>>>
>>> These tests have been verified through the Mach5 and jprt systems.
>>>
>>> Thanks,
>>> Jini.
>>>
>