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

[Staffan Larsen]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131030/8237b069/attachment.html