The reduction in testsuites in JdpTest.sh is due to those "testsuites" removed being ported to Java with this change. The last "testsuite" in JdpTest.sh however, has not been ported yet, therefore JdpTest.sh can't be removed completely.

Mikael

On 2013-11-11 14:19, Staffan Larsen wrote:
Why are there changes to the existing tests? Most look like whitespace
changes, but JdpTest.sh changes the “testsuites” that are being run.

/Staffan

On 08 Nov 2013, at 18:45, Mikael Auno <mikael.a...@oracle.com
<mailto:mikael.a...@oracle.com>> wrote:

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>
<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