Alex, This looks much better! Thanks for spending the time.
README: nit: contribuited -> contributed JdpTestUtilTest.java: Can you add a comment saying this is a test for the tests - just like PacketTest.java and PortAlreadyInUseTest.java? PortFinder.java: Please remove this and use Utils.getFreePort() since ykantser's fix is now pushed. Thanks, /Staffan On 29 okt 2013, at 15:53, Alex Schenkman <alex.schenk...@oracle.com> wrote: > Hi Staffan, > > There is a new webrev here [1], addressing your comments. > > The Jdp specs have changed a bit, adding three optional fields to the Jdp > packet [2]. > The JEP-168 [3] is not updated yet, but Dmitry will do it soon. > This enhancement request is tracking the changes needed for SQE [4]. > > See inlined answers for details on your previos comments, please. > > Thanks! > > > [1] > http://cr.openjdk.java.net/~dsamersoff/sponsorship/aschenkman/8014506/webrev.04/ > [2] https://bugs.openjdk.java.net/browse/JDK-8004213 > [3] http://openjdk.java.net/jeps/168 > [4] https://bugs.openjdk.java.net/browse/JDK-8027436 > > On 2013-10-24 10:54, Staffan Larsen wrote: >> Alex, >> >> test/sun/management/jdp/README >> Thanks for providing a README! >> References to JDK-7169888 seem incorrect to me. >> L8: "written by" appears twice. >> L11: "defautl" -> "default" >> > Fixed. >> test/lib/testlibrary/jdk/testlibrary/PortFinder.java >> This should be coordinated with the review for 8022229 which contains the >> same code in a different testlibrary class: >> http://cr.openjdk.java.net/~ykantser/8022229/webrev.01/test/lib/testlibrary/jdk/testlibrary/Utils.java.html >> > I made a private copy of this function, and have marked it as deprecated. > As soon as ykanster's code is merged I can delete my private function and use > the library. > The reason for this is to get this patch pushed without further delay. > >> test/sun/management/jdp/7169888/JdpClient.java >> test/sun/management/jdp/7169888/JdpDoSomething.java >> test/sun/management/jdp/7169888/JdpTest.sh >> test/sun/management/jdp/7169888/JdpUnitTest.java >> These were moved to a subdirectory, but these tests have nothing to do >> with bug 7169888, so why that name? In any case we should avoid naming test >> directories after bug ids, and instead use a descriptive name. >> > This bug number was wrong. These tests were now moved back to the jdp folder. > I have expanded the README file with some details about these tests. > > Dmitry's shell-based are as follows: > test_01 - basic test, check if JDP packet assembler and other > parts of JDP is not broken > > test_02 - test if JDP starts with custom parameters. > > test_03 - test if jcmd is able to start jdp with > custom parameters (disabled) > > test_04 - test if JDP starts with default parameters > > test_05 - test if jcmd is able to start jdp with default > parameters (disabled) > > > test_03 and test_05 are diabled becuase there is a mismatch between hotsport > and jdk repos, according to Dmitry. These cases (jcmd) are not covered by > this patch, and are part of the test enhacements [4]. > > test_01 does partly overlap with the Java-based tests, but we should keep it > until we implement the latest enhacements [4]. > > >> test/sun/management/jdp/ClientConnection.java >> No comments. >> >> test/sun/management/jdp/DefaultLauncher.java >> Can "@library ../../../lib/testlibrary" be replaced by "@library >> /lib/testlibrary" ? >> > Fixed. >> test/sun/management/jdp/OffLauncher.java >> Can "@library ../../../lib/testlibrary" be replaced by "@library >> /lib/testlibrary" ? >> > Fixed. > >> test/sun/management/jdp/SpecificJdpAddressLauncher.java >> Can "@library ../../../lib/testlibrary" be replaced by "@library >> /lib/testlibrary" ? >> > Fixed. >> test/sun/management/jdp/DynamicLauncher.java >> No comments. >> > Fixed. > >> test/sun/management/jdp/JdpOffTest.java >> For the future: it is customary that the file containing the jtreg >> directives be named xxxTest.java, and supporting files not ending in Test. >> > I have renamed the tests following your recomendation. > Jtreg tests have now the test suffix. > >> test/sun/management/jdp/JdpOnTest.java >> For the future: it is customary that the file containing the jtreg >> directives be named xxxTest.java, and supporting files not ending in Test. >> > Same as above. >> test/sun/management/jdp/JdpTest.java >> No comments. >> >> test/sun/management/jdp/JdpTestUtil.java >> No comments. >> >> test/sun/management/jdp/JdpTestUtilTest.java >> test/sun/management/jdp/PacketTest.java >> test/sun/management/jdp/PortAlreadyInUseTest.java >> These are tests for the tests. But where are the tests for the tests for >> the tests? :) >> Not reviewed. >> >> >> Thanks, >> /Staffan >> >
