Rename JstatdTest.getToolOptions() to JstatdTest.getDestination(). nit: JstatdTest.cleanUpThread() shouldn't be static.
Thanks, /Staffan On 28 okt 2013, at 17:12, Yekaterina Kantserova <yekaterina.kantser...@oracle.com> wrote: > Hi, > > http://cr.openjdk.java.net/~ykantser/8022229/webrev.03/ > > Moved ToolConfig functionality to JstatdTest (renamed JstatdHelper) and > deleted ToolConfig. The tests are also changed a little: > > public class TestJstatdDefaults { > > public static void main(String[] args) throws Throwable { > JstatdTest test = new JstatdTest(); > test.doTest(); > } > > } > > Hope it looks better now. > > Thanks, > Katja > > ----- Ursprungligt meddelande ----- > Från: staffan.lar...@oracle.com > Till: yekaterina.kantser...@oracle.com > Kopia: serviceability-dev@openjdk.java.net, david.hol...@oracle.com, > jaroslav.bacho...@oracle.com,christian.tornqv...@oracle.com, > hotspot-...@openjdk.java.net, mattis.casteg...@oracle.com > Skickat: måndag, 28 okt 2013 12:57:38 GMT +01:00 > Amsterdam/Berlin/Bern/Rom/Stockholm/Wien > Ämne: Re: RFR (S): 8022229: Intermittent test failures in sun/tools/jstatd > > 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
