On Wed, 21 Oct 2020 16:38:16 GMT, Chris Hegarty <che...@openjdk.org> wrote:
>> Michael McMahon has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 36 additional >> commits since the last revision: >> >> - Merge branch 'master' into unixdomainchannels >> - updated one error in review >> - further test update from Daniel >> - forgot to stage updated test files in last commit >> - update from Daniel's test comments >> - Merge branch 'master' into unixdomainchannels >> - final feedback from Alan >> - Merge branch 'master' into unixdomainchannels >> - - Alan's review patch from Oct 18 >> - Reverted changes to JFR unit tests >> - - added NullTest and fix in SocketChannel.open >> - updated test bug ids >> - ... and 26 more: >> https://git.openjdk.java.net/jdk/compare/d3e47796...01d7cf53 > > test/jdk/java/nio/channels/unixdomain/Bind.java line 51: > >> 49: if (!supported()) { >> 50: System.out.println("Unix domain channels not supported"); >> 51: return; > > Here it would be better to throw the `jtreg.SkippedException`, rather than > silently returning. Same comment for any other tests that check > supported-ness before continuing to execute. Ok > test/jdk/java/nio/channels/unixdomain/Bind.java line 66: > >> 64: SocketChannel.open(StandardProtocolFamily.UNIX).close(); >> 65: } catch (UnsupportedOperationException e) { >> 66: return false; > > In this case `ServerSocketChannel.open(UNIX)` should also throw UOE. I'll add a specific test that checks if `SocketChannel.open(UNIX)` throws UOE than `ServerSocketChannel.open(UNIX)` should also. > test/jdk/java/nio/channels/unixdomain/policy1 line 1: > >> 1: grant { > > It is often the case that a copyright and license header is added to such > policy files, policy1, policy2, policy3 OK > test/jdk/java/nio/channels/unixdomain/NonBlockingAccept.java line 59: > >> 57: serverSocketChannel.bind(null); >> 58: SocketChannel socketChannel = serverSocketChannel.accept(); >> 59: System.out.println("The socketChannel is : expected Null " + >> socketChannel); > > The null-ness of the returned socket channel be asserted, regardless of > whether or not a prior version of the code threw an exception. OK > test/jdk/java/nio/channels/unixdomain/IOExchanges.java line 49: > >> 47: >> 48: public class IOExchanges { >> 49: static boolean supported = true; > > This should really be `unixDomainSupported` or some such, to indication that > it us used to seed the set of protocol families to test with; IP-only, or > IP+Unix. When I wrote this test originally, the intent was that it could > operate with channels of different protocol types, IP, rsocket, and now Unix. > Happy to see that it is finally being integrated. Maybe update the copyright > year range to start at 2018, see > https://cr.openjdk.java.net/~chegar/rsocket/IOExchanges/test/jdk/jdk/net/RdmaSockets/rsocket/SocketChannel/IOExchanges.java.html Ok, thanks for review! ------------- PR: https://git.openjdk.java.net/jdk/pull/52
