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.schenk...@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.lar...@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.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 >>>> >>> >> >> -- >> Alex Schenkman >> Java VM SQE Stockholm > > -- > Alex Schenkman > Java VM SQE Stockholm
