Hi,
I've done following big changes after the Staffan's review:
- ported JDKToolLauncher, JDKToolFinder, Plattform from hotspot
testlibrary (http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot) for
re-using in ToolConfig
- changed "verifiy output form jps, jstat"-methods to ignore warnings
from VM
- tools will be launched without -vmoptions
plusmade fixes for all minor comments.
If it's a good idea to push testlibrary changes separately I can make a
separate patch for them.
Webrev:
http://cr.openjdk.java.net/~ykantser/8022229/webrev.02/
Primal bug:
https://bugs.openjdk.java.net/browse/JDK-8022229
Similar bugs:
https://bugs.openjdk.java.net/browse/JDK-8019630
https://bugs.openjdk.java.net/browse/JDK-6636094
https://bugs.openjdk.java.net/browse/JDK-6543979
Thanks,
Katja
On 10/23/2013 12:55 PM, Staffan Larsen wrote:
test/lib/testlibrary/jdk/testlibrary/Asserts.java
Same code as already exists in the hotspot testlibrary.
No further comments.
test/lib/testlibrary/jdk/testlibrary/ProcessThread.java
L33: stating -> starting
L66: staring -> starting
test/lib/testlibrary/jdk/testlibrary/TestThread.java
This code comes from an internal test library.
No further comments.
test/lib/testlibrary/jdk/testlibrary/Utils.java
No comments.
test/lib/testlibrary/jdk/testlibrary/XRun.java
This code comes from an internal test library.
No further comments.
test/sun/tools/jstatd/JstatGcutilParser.java
The parse() method could be made more robust by discarding any lines
that come before the header lines. This can happen if the JVM outputs
a warning message for some reason before running jstat.
I don't like the special-case for the CCS column in verify() - took
me a while to find it. Should we add a new enum called
PERCENTAGE_OR_DASH to handle that instead?
L67: getType() should be private.
test/sun/tools/jstatd/JstatdHelper.java:
L54: Does verifyJpsOutput() work correctly with output of the form:
1234 -- main class information unavailable
1235 -- process information unavailable
Also: same comment here as for JstatGcutilParser.java: sometimes the
JVM outputs warning messages before the output from the tool.
L46: "attach" is probably not the best way to describe the operation
of jps. It does not attach to the process, it merely lists the running
processes. Perhaps runJps() is a better name?
test/sun/tools/jstatd/TestJstatdDefaults.java
test/sun/tools/jstatd/TestJstatdExternalRegistry.java
test/sun/tools/jstatd/TestJstatdPort.java
test/sun/tools/jstatd/TestJstatdPortAndServer.java
test/sun/tools/jstatd/TestJstatdServer.java
No comments.
test/sun/tools/jstatd/TestJstatdUsage.java
Same comment here as for JstatGcutilParser.java: sometimes the JVM
outputs warning messages before the output from the tool.
test/sun/tools/jstatd/ToolConfig.java
ToolConfig + Utils.get(Forward)VmOptions() seem to be doing similar
things to JDKToolLauncher in the hotspot testlibrary. It is
unfortunate if we have two ways to do similar things in the two
different testlibraries. Would it be possible to reuse the hotspot
code here instead?
You have also changed the test. Previously the tools were not
launched with the jtreg -vmoptions (test.vm.opts), whereas now they
will be. Is this intentional?
General comments:
Can't we do:
* @library /lib/testlibrary
instead of
* @library ../../../lib/testlibrary
?
It seems that at least some of the functionality should be reused for
re-writing the jstat and jps tests as well. I guess we can look at
that at a later time, though.
While I applaud the move from shell scripts to Java code, I can't
shake the feeling that the shell scripts were easier to read and
follow. The Java code is much more verbose and spread out over
multiple helpers and libraries, making it harder to grasp. That may be
the price we have to pay, though.
Thanks,
/Staffan
