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

[Alex Schenkman]

Hi Staffan,

Any chance you can review this again?
http://cr.openjdk.java.net/~dsamersoff/sponsorship/aschenkman/8014506/ 
<http://cr.openjdk.java.net/%7Edsamersoff/sponsorship/aschenkman/8014506/>

Thanks in advance!

On 2013-09-02 09:56, Alex Schenkman wrote:
> I've added a README file and moved Dmitry's tests into its own folder, 
> to address the comments below.
> http://cr.openjdk.java.net/~dsamersoff/sponsorship/aschenkman/8014506/webrev.03/ 
> <http://cr.openjdk.java.net/%7Edsamersoff/sponsorship/aschenkman/8014506/webrev.03/>
>
> On 2013-08-21 20:10, Staffan Larsen wrote:
>> A couple of more comments:
>>
>> - It was a little too hard to figure out exactly how the tests 
>> worked: who is sending JDP packets? who is listening for them? who is 
>> launching the tests? Perhaps it would make sense to add a small 
>> readme which explains how it all fits together?
>>
>> - There are already some JDP tests in that directory which are also 
>> made up of a couple of files. Mixing them together makes it hard to 
>> see which files belong to which set of tests. Maybe we should have 
>> distinct subfolders for the sets?
>>
>> - There is also the question of how much overlap there are between 
>> your tests and the already existing tests. It would be preferable to 
>> have no overlap. Perhaps some of the existing shell-based tests can 
>> be subsumed by your java-based tests?
>>
>> Thanks,
>> /Staffan
>>
>> On 21 aug 2013, at 18:15, Staffan Larsen <staffan.larsen at oracle.com 
>> <mailto:staffan.larsen at oracle.com>> wrote:
>>
>>> 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 
>>> <mailto: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/ 
>>>>> <http://cr.openjdk.java.net/%7Edsamersoff/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
>>>
>>
>
> -- 
> 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/20131023/f1e4f591/attachment.html