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

[Alex Schenkman]

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/20131108/e33353c9/attachment-0001.html