Hi Chris,
thank you for your comments. Please see updated review. (I left braces
in one line "if" blocks, hope it wasn't strong objection)
http://cr.openjdk.java.net/~snikandrova/8136933/webrev.04/
<http://cr.openjdk.java.net/%7Esnikandrova/8136933/webrev.04/>
Tested as standalone 1 test manual run and with JPRT.
Thank you,
Svetlana
On 24.05.2016 21:59, Chris Hegarty wrote:
On 24 May 2016, at 17:21, Svetlana Nikandrova <svetlana.nikandr...@oracle.com>
wrote:
I hate being annoying, but may be you can find another minute to review updated
diff?
This mainly look fine. Some comments:
- The test will need to '@build jdk.testlibrary.*’ to ensure that the library
classes
are compiled. Please verify that the test can run successfully, by itself,
in a
clean work directory.
- You can initialize ‘expectSupport' and make it final:
private static final boolean expectSupport = checkExpectedOptionSupport();
- If you statically import SO_FLOW_SLA, then things will fit easier, e.g.
import static jdk.net.ExtendedSocketOptions.SO_FLOW_SLA;
…
if (s.supportedOptions().contains(SO_FLOW_SLA) != expectSupport) {
… and maybe drop the unnecessary braces.
- Can you please revert the indentation on SocketFlow.create(),
i.e. line up the dots
-Chris.
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.
