There was some unintended whitespace changes in webrev.05 that Alex had missed. I removed them and uploaded webrev.06 for him.

http://cr.openjdk.java.net/~miauno/8014506/webrev.06/

Mikael

On 2013-11-08 17:02, Alex Schenkman wrote:
Hi list,

Latest version is up for review here [1].
It fixes Staffan's latest comments (see below), as well as some
indentation and tab/spaces nits.

Thanks!

[1] http://cr.openjdk.java.net/~miauno/8014506/
<http://cr.openjdk.java.net/%7Emiauno/8014506/>

On 2013-10-30 16:03, Staffan Larsen wrote:
Alex,

This looks much better! Thanks for spending the time.

README:
  nit: contribuited -> contributed

JdpTestUtilTest.java:
  Can you add a comment saying this is a test for the tests - just
like PacketTest.java and PortAlreadyInUseTest.java?

PortFinder.java:
  Please remove this and use Utils.getFreePort() since ykantser's fix
is now pushed.

Thanks,
/Staffan

On 29 okt 2013, at 15:53, Alex Schenkman <alex.schenk...@oracle.com
<mailto:alex.schenk...@oracle.com>> wrote:

Hi Staffan,

There is a new webrev here [1], addressing your comments.

The Jdp specs have changed a bit, adding three optional fields to the
Jdp packet [2].
The JEP-168 [3] is not updated yet, but Dmitry will do it soon.
This enhancement request is tracking the changes needed for SQE [4].

See inlined answers for details on your previos comments, please.

Thanks!


[1]
http://cr.openjdk.java.net/~dsamersoff/sponsorship/aschenkman/8014506/webrev.04/
<http://cr.openjdk.java.net/%7Edsamersoff/sponsorship/aschenkman/8014506/webrev.04/>
[2] https://bugs.openjdk.java.net/browse/JDK-8004213
[3] http://openjdk.java.net/jeps/168
[4] https://bugs.openjdk.java.net/browse/JDK-8027436

On 2013-10-24 10:54, Staffan Larsen wrote:
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"

Fixed.
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
<http://cr.openjdk.java.net/%7Eykantser/8022229/webrev.01/test/lib/testlibrary/jdk/testlibrary/Utils.java.html>

I made a private copy of this function, and have marked it as deprecated.
As soon as ykanster's code is merged I can delete my private function
and use the library.
The reason for this is to get this patch pushed without further delay.

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.

This bug number was wrong. These tests were now moved back to the jdp
folder.
I have expanded the README file with some details about these tests.

Dmitry's shell-based are as follows:
test_01 - basic test, check if JDP packet assembler and other
           parts of JDP is not broken

test_02 - test if JDP starts with custom parameters.

test_03 - test if jcmd is able to start jdp with
           custom parameters (disabled)

test_04 - test if JDP starts with default parameters

test_05 - test if jcmd is able to start jdp with default
           parameters (disabled)


test_03 and test_05 are diabled becuase there is a mismatch between
hotsport and jdk repos, according to Dmitry. These cases (jcmd) are
not covered by this patch, and are part of the test enhacements [4].

test_01 does partly overlap with the Java-based tests, but we should
keep it until we implement the latest enhacements [4].


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

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

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

Fixed.

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

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

Fixed.

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.

I have renamed the tests following your recomendation.
Jtreg tests have now the test suffix.

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.

Same as above.
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




--
Alex Schenkman
Java VM SQE Stockholm


Reply via email to