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

[Staffan Larsen]

Alex,

This looks a lot better than the previous version. Thanks for cleaning things up. Below are a couple of comments:

- Use of "package" is discouraged for jtreg test: http://openjdk.java.net/jtreg/faq.html#question4.9

- Maybe change name of SpecificLauncher.java to SpecificJdpAddressLauncher.java?

- The constructor and onSocketTimeOut() in JdpOffTest.java has wrong indentation.

- The comment for JdpOffTest.shouldContinue() has weird indentation.

- How does one run PacketTest.java? It looks like a TestNG test, but I couldn't get jtreg to run it. 

- Why the mix of standard jtreg tests and TestNG tests?

- nit: somewhat non-standard formatting with spaces inside parens: socket.receive( datagram );

- nit: statements should be on a new line:
            if ( hasTestLivedLongEnough() ) { shutdown(); }


Thanks,
/Staffan

On 21 aug 2013, at 14:01, Alex Schenkman <alex.schenkman at oracle.com> wrote:

> Staffan, do you think you can take another look this?
> Thanks!
> 
> On 2013-07-04 14:26, Alex Schenkman wrote:
>> Thanks Staffan for your comments.
>> 
>> The indentation is corrected in all files, sorry about that.
>> 
>> About the racing condition in PortFinder.java, you are absolutely right, there is no warranty that this port will be free.
>> I do not know a better way to do this. Any suggestions?
>> On the other hand, even if this fails every now and then, it will work most of the times and it is better than what we have today.
>> I have added better comments in PortFinder to reflect the problem.
>> 
>> The new webrev is here [1], at the same address than before.
>> 
>> Thanks!
>> 
>> [1] http://cr.openjdk.java.net/~dsamersoff/8014506/webrev.01/
>> 
>> PS: 
>> PortFinder.java, line 37:
>> There is an extra <p/> tag that should be removed.
>> 
>> 
>> 
>> On 2013-06-26 13:54, Staffan Larsen wrote:
>>> test/lib/testlibrary/jdk/testlibrary/PortFinder.java:
>>> 
>>> - This looks racy to me. There is no guarantee that the port will still be available after the call to close().
>>> - Missing copyright notice.
>>> - In comment "on of them" -> "one of them"
>>> 
>>> test/sun/management/jdp/ClientConnection.java:
>>> 
>>> - broken indentation
>>> - inconsistent spacing in method calls. Sometime "joinGroup(address);" and sometimes "setSoTimeout( msTimeOut );". Should be without spaces.
>>> 
>>> test/sun/management/jdp/DynamicLauncher.java:
>>> 
>>> - type: binded -> bound
>>> - inconsistent spacing in method calls
>>> 
>>> test/sun/management/jdp/JdpOffTest.java
>>> 
>>> - broken indentation
>>> 
>>> test/sun/management/jdp/JdpTestUtil.java
>>> 
>>> - broken indentation
>>> 
>>> 
>>> I stopped reading here. Can we get these basic things fixed before the next review round, please?
>>> 
>>> /Staffan
>>> 
>>> 
>>> On 26 jun 2013, at 12:21, Dmitry Samersoff <Dmitry.Samersoff at oracle.com> wrote:
>>> 
>>>> Nils,
>>>> 
>>>> I'm sponsoring this push but don't see any review replies in this thread.
>>>> 
>>>> Webrev refreshed against latest tl is here:
>>>> 
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8014506/webrev.01/
>>>> 
>>>> -Dmitry
>>>> 
>>>> 
>>>> On 2013-05-27 12:25, Alex Schenkman wrote:
>>>>> Please review these tests for the Jdp protocol.
>>>>> 
>>>>> There are three cases:
>>>>> 1) Jdp is off.
>>>>> 2) Jdp is on and using default address and port.
>>>>> 3) Jdp is on using custom address and port.
>>>>> 
>>>>> Tests: http://cr.openjdk.java.net/~nloodin/8014506/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Enloodin/8014506/webrev.00/>
>>>>> Jdp feature enhancement: https://jbs.oracle.com/bugs/browse/JDK-8002048
>>>>> JDK CCC: http://ccc.us.oracle.com/8002048
>>>>> 
>>>>> 
>>>>> Thanks in advance!
>>>>> 
>>>>> -- 
>>>>> Alex Schenkman
>>>>> Java VM SQE Stockholm
>>>>> 
>>>> -- 
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the sources.
>> 
>> -- 
>> Alex Schenkman
>> Java VM SQE Stockholm
> 
> -- 
> Alex Schenkman
> Java VM SQE Stockholm

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130821/926c0e76/attachment.html