This looks a lot better. Just few more comments: ToolConfig.java L72 - "getToolOptins" -> "getToolOptions" L35 - why is port stored as a String? Also, could be renamed to rmiRegistryPort to clarify what it is for.
Why are some tool-specific options added in getToolOptins() and some in the getJpsCmd()/getJstatdCmd()/... methods? Maybe changing getToolOptins() to only return the host part of the options and moving the rest of the setup to getJpsCmd()/getJstatdCmd()/...? Thanks, /Staffan On 24 okt 2013, at 15:57, Yekaterina Kantserova <yekaterina.kantser...@oracle.com> wrote: > 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 > > plus made 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 >
