RFR 8014506: Test for Jdp (Java Discovery Protocol) feature

[Staffan Larsen]

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.auno at 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.schenkman at oracle.com
>>> <mailto:alex.schenkman at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131111/2c20ea15/attachment-0001.html