RFR (S): 8022229: Intermittent test failures in sun/tools/jstatd

[Yekaterina Kantserova]

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