Thanks for the explanation. Reviewed! I will sponsor the push.
And thanks for hanging in there during the review process. /Staffan On 11 Nov 2013, at 14:57, Mikael Auno <mikael.a...@oracle.com> wrote: > The reduction in testsuites in JdpTest.sh is due to those "testsuites" > removed being ported to Java with this change. The last "testsuite" in > JdpTest.sh however, has not been ported yet, therefore JdpTest.sh can't be > removed completely. > > Mikael > > On 2013-11-11 14:19, Staffan Larsen wrote: >> Why are there changes to the existing tests? Most look like whitespace >> changes, but JdpTest.sh changes the “testsuites” that are being run. >> >> /Staffan >> >> On 08 Nov 2013, at 18:45, Mikael Auno <mikael.a...@oracle.com >> <mailto:mikael.a...@oracle.com>> wrote: >> >>> There was some unintended whitespace changes in webrev.05 that Alex >>> had missed. I removed them and uploaded webrev.06 for him. >>> >>> http://cr.openjdk.java.net/~miauno/8014506/webrev.06/ >>> >>> Mikael >>> >>> On 2013-11-08 17:02, Alex Schenkman wrote: >>>> Hi list, >>>> >>>> Latest version is up for review here [1]. >>>> It fixes Staffan's latest comments (see below), as well as some >>>> indentation and tab/spaces nits. >>>> >>>> Thanks! >>>> >>>> [1]http://cr.openjdk.java.net/~miauno/8014506/ >>>> <http://cr.openjdk.java.net/%7Emiauno/8014506/> >>>> >>>> On 2013-10-30 16:03, Staffan Larsen wrote: >>>>> 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 >>>>> <mailto:alex.schenk...@oracle.com> >>>>> <mailto: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/ >>>>>> <http://cr.openjdk.java.net/%7Edsamersoff/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 >>>>>>> <http://cr.openjdk.java.net/%7Eykantser/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 >>>>>>> >>>>>> >>>>> >>>> >>>> -- >>>> Alex Schenkman >>>> Java VM SQE Stockholm
