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