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> 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>> 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
