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

[Staffan Larsen]

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


On 22 okt 2013, at 10:24, Yekaterina Kantserova <yekaterina.kantserova at oracle.com> wrote:

> Sorry, the wrong webrev link has slipped through.
> 
> Here is the right one:
> 
> http://cr.openjdk.java.net/~ykantser/8022229/webrev.01/
> 
> Thanks,
> Katja
> 
> 
> 
> On 10/21/2013 05:23 PM, Yekaterina Kantserova wrote:
>> Hi,
>> 
>> I've done following changes after the reviews:
>> - added open copyrights
>> - documented all public functions in jdk.testlibrary.TestThread, jdk.testlibrary.XRun, jdk.testlibrary.ProcessThread, jdk.testlibrary.Utils
>> - merged jdk.testlibrary.Asserts from hotspot testlibrary
>> 
>> Jaroslav's comments:
>> - test/sun/tools/jstatd/JstatdHelper.java 82-84 - is deleted, different exception can show up there while waiting until jstatd is up and running; better just look for valid pid;
>> - test/sun/tools/jstatd/JstatdHelper.java clean up processes - since jstatd process is daemon, the "clean up" is to kill the target process. Please look at the changed JstatdHelper and tell me if it's still unclear.
>> - test/lib/testlibrary/jdk/testlibrary/Utils.java serverSocket.close() should be called in "finally" - fixed
>> 
>> Webrev: 
>> http://cr.openjdk.java.net/~ykantser/8022229/webrev.01/
>> 
>> 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
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131023/93522d7f/attachment-0001.html