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

Staffan Larsen staffan.larsen at oracle.com
Thu Oct 24 01:54:13 PDT 2013 

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"

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

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.

test/sun/management/jdp/ClientConnection.java
  No comments.

test/sun/management/jdp/DefaultLauncher.java
  Can "@library ../../../lib/testlibrary" be replaced by "@library /lib/testlibrary" ?

test/sun/management/jdp/OffLauncher.java
  Can "@library ../../../lib/testlibrary" be replaced by "@library /lib/testlibrary" ?

test/sun/management/jdp/SpecificJdpAddressLauncher.java 
  Can "@library ../../../lib/testlibrary" be replaced by "@library /lib/testlibrary" ?

test/sun/management/jdp/DynamicLauncher.java
  No comments.

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.

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.

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

On 23 okt 2013, at 14:51, Alex Schenkman <alex.schenkman at oracle.com> wrote:

> Hi Staffan,
> 
> Any chance you can review this again?
> http://cr.openjdk.java.net/~dsamersoff/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/
>> 
>> 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> 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> 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
>>>> 
>>> 
>> 
>> -- 
>> 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/20131024/7440f88d/attachment-0001.html

More information about the serviceability-dev mailing list