RFR 8241305: Add protocol specific factory creation methods to SocketChannel and ServerSocketChannel

[Michael McMahon]

On 14/05/2020 19:02, Alan Bateman wrote:
> On 13/05/2020 22:51, Michael McMahon wrote:
>> I have updated the webrev for this at 
>> http://cr.openjdk.java.net/~michaelm/8241305/webrev.4/
>> based on the last round of comments.
> LocalSocketAddressType - looks good but not clear why it re-runs with 
> preferIPv6Addresses=true.
>
Ok, I overlooked that. Thanks
> OpenAndConnect - looks much better, a few comments:
>
> The use of ignoreTest in each of the parameterized tests is a bit of 
> anti-pattern. The data provider should filter the test cases so that 
> the test aren't invoked with untestable parameters. That should avoid 
> lots of skipped tests too.
>
Initially I was thinking it wouldn't be any clearer
because a predicate might need to be applied to each row of the table
to determine if a local IPv4 address or local IPv6 address is present on 
this
particular execution.

But, it may be possible to split it into two separate tables for IPv4 
and IPv6.
If that works, I will do that.

All remaining comments I agree with. I'll have another webrev later today

Thanks again,

Michael.

> There are several places where exceptions are being caught and the 
> error(Exception) method used to re-thrown them. I think 
> error(Exception) can be removed and just let TestNG handle the failures.
>
> There are a few duplicate tests: L138 and L139, L144 and L145, L195 
> and L196.
>
> The tests that check for DONT_BIND would be a bit clear if they 
> created "csa" after caddr != DONT_BIND, e.g.
>
> if (caddr != DONT_BIND) {
>     dc.bind(getSocketAddress(caddr);
> }
>
>
> ProtocolFamilies - looks much better, a few comments:
>
> isWindows can be removed.
>
> I think the data providers could be a bit clearer if they used if 
> (hasIPv6 && !preferIPv4) { ... }.
> Do you intend to keep the commented out test cases in the openConnect 
> data provider?
>
> getLocalIPv6Address looks like it could return a loopback address, 
> filter step missing? BTW: if you change ia4 and ia6 to be InetAddress 
> then it should eliminate the casts in all places.
>
> scOpenConnect, the connectOp isn't really needed, simpler to just call 
> sc.connect if expectedException is null.
>
> dcOpen names the parameter "sfamily", typo? The other methods use 
> "family".
>
>
> I think that is all I have for this round.
>
> -Alan.
>
>