Looks good! Reviewed. Thanks, /Staffan
On 30 okt 2013, at 10:27, Yekaterina Kantserova <yekaterina.kantser...@oracle.com> wrote: > > http://cr.openjdk.java.net/~ykantser/8022229/webrev.04/ > > Fixed your points and changed ./test/sun/tools/jstatd/TestJstatdUsage.java to > use JDKToolLauncher instead of JDKFinder. > > Thanks, > Katja > > > On 10/28/2013 07:02 PM, Staffan Larsen wrote: >> 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 >> > > <8022229.4.patch>
