I hate being annoying, but may be you can find another minute to review
updated diff?
Thank you,
Svetlana
On 23.05.2016 19:36, Svetlana Nikandrova wrote:
Alan, Chris,
thank you for your comments. I've decided to do as Chris suggested and
updated existing test test/jdk/net/Sockets/Test.java instead of
creating a new one.
Please see updated review:
http://cr.openjdk.java.net/~snikandrova/8136933/webrev.03/
<http://cr.openjdk.java.net/%7Esnikandrova/8136933/webrev.03/>
Thank you,
Svetlana
On 20.05.2016 18:19, Chris Hegarty wrote:
On 20 May 2016, at 14:10, Alan Bateman <alan.bate...@oracle.com> wrote:
On 20/05/2016 14:05, Svetlana Nikandrova wrote:
Alan,
another test related to this option is on the same level
(test/jdk/net/SocketFlow)
I added this recently, when working on a different issue, when I
noticed that there were no tests for the SocketFllow API, regardless
of the underlying system support. The test directory structure
matches the class package/name hierarchy.
so I thought it's ok to maintain the same hierarchy. I can move
test to test/jdk/net/Sockets/ExtendedSocketOptions/SO_FLOW_SLA or
to test/jdk/net/Sockets, but in that case won't it be a little bit
confusing?
As for overlapping coverage: existing test silently exits if option
is not in supported list while this one is focused on platform
support check.
I think it would be good to put all the tests in the same place,
will make it easier for further maintenance in this area.
The proposed new test seems, somewhat, to be a replacement for
test/jdk/net/Sockets/Test.java, but it does not assert that if set
succeeds that get should return something useful? Also set
typically cannot succeed unless run with privileges. The test also
seems to focus on setting the option through the idk.net.Sockets API
rather than through the setOption of the socket types, any reason
for this? The jdk.net.Sockets API should be deprecated in 9, as
we have a standard replacement.
When working in the area previously, I ran
test/jdk/net/Sockets/Test.java manually and examined the output
to determine that the option was being set correctly.
It this test is merely to check that the option is supported on
S 11.2 +, then maybe that could be added to the existing
test/jdk/net/Sockets/Test.java ?
-Chris.
