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.schenk...@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.samers...@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
