Thank you very much, David.
- Jini.
On 11/16/2017 8:16 AM, David Holmes wrote:
On 15/11/2017 3:27 PM, Jini George wrote:
Thank you, Sharath and David. The revised webrev after addressing the
comments are at:
http://cr.openjdk.java.net/~jgeorge/8190307/webrev.03/
Please put all the informational tags first:
@test
@bug
@summary
You didn't delete the
int exitValue = p.exitValue();
lines.
No need to see an updated webrev with these fixes.
Thanks,
David
Thanks,
Jini.
On 11/15/2017 7:00 AM, David Holmes wrote:
Hi Jini,
In addition to Sharath's comments please add @bug and @summary items
to each test header.
Thanks,
David
-----
On 15/11/2017 2:31 AM, Sharath Ballal wrote:
Hi Jini,
Code looks good. Some nits.
TestUniverse.java:
These lines can be removed
26 import java.io.File;
33 import jdk.test.lib.process.ProcessTools;
84 int exitValue = p.exitValue();
TestType.java:
These lines can be removed
32 import jdk.test.lib.process.ProcessTools;
86 int exitValue = p.exitValue();
You may want to add few more strings in the defaultOutputStrings.
TestIntConstant.java
These lines can be removed
32 import jdk.test.lib.process.ProcessTools;
89 int exitValue = p.exitValue();
You may want to add few more strings in the defaultOutputStrings.
Thanks,
Sharath (not a reviewer)
-----Original Message-----
From: Jini George
Sent: Tuesday, November 14, 2017 2:19 PM
To: David Holmes; serviceability-dev@openjdk.java.net
Subject: Re: RFR: (small): JDK-8190307: SA: Sanity tests for the
clhsdb commands: universe, intconstant, type
Thank you very much, David. Here is the revised webrev:
http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/
Have reduced the exitValue checking fragment and included
p.destroyForcibly(). Also placed the creation of OutputAnalyzer
before waiting for the jhsdb process exit, to avoid jhsdb hangs due
to the output stream buffer not being consumed. (Thanks, Sharath!)
Thanks,
Jini.
On 11/6/2017 12:58 PM, David Holmes wrote:
Hi Jini,
On 3/11/2017 8:51 PM, Jini George wrote:
Here is the updated webrev:
http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
I have made changes to validate the test results of each command
separately, done away with the asserts and have added some more
comments.
This looks much better to me - thanks. This fragment:
int exitValue = p.exitValue();
OutputAnalyzer output = new OutputAnalyzer(p);
System.out.println(output.getOutput());
if (exitValue != 0) {
throw new Error("clhsdb wasn't run successfully.");
}
can be reduced to:
OutputAnalyzer output = new OutputAnalyzer(p);
output.shouldHaveExitValue(0);
There's probably no need to print getOutput() as if anything goes
wrong it will be printed by OutputAnalyzer anyway. But that's your
choice.
It may also be advisable to ensure the process is destroyed if you get
an exception here:
try {
p.waitFor();
} catch (InterruptedException ie) {
+ p.destroyForcibly();
throw new Error("Problem awaiting the child process:
" +
ie, ie);
}
similar to how ProcessTools.executeProcess manages things.
Thanks,
David
-----
Thank you,
Jini.
On 10/31/2017 12:32 PM, Jini George wrote:
Thank you for the quick review, David. My comments inline:
On 10/30/2017 11:18 AM, David Holmes wrote:
Plus this assumes G1 is being used but Lingered App is started
using the test run vmOptions AFAICS so it would use whatever GC
were passed through, wouldn't it?
Okay the above are just examples of "int constants" that will be
printed when you dump all the "int constants". The G1 constant
doesn't indicate (I presume) that G1 is the active GC.
Yes, the int constants contain compile time values and there are
entries for all the GC types.
As I said to Sharath it would be good to validate the results of
each command separately rather than collectively.
Will do.
Thanks!
- Jini.
Thanks,
David
---
TestType.java
The expected output is not quite so strange, but still could do
with some commentary.
90 Asserts.assertTrue(output.contains("type
G1CollectedHeap CollectedHeap"));
Same comment about assuming G1.
Thanks,
David
------
On 30/10/2017 1:44 PM, Jini George wrote:
Hello,
We have been working on writing sanity tests for the various
jhsdb clhsdb commands of the SA to improve the robustness of the
SA. As a part of this, here is a webrev for sanity tests for 3 of
the clhsdb commands:
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
I would like to request for reviews for this simple addition.
Thank you,
Jini.
