zentol commented on a change in pull request #17227:
URL: https://github.com/apache/flink/pull/17227#discussion_r705915129
##########
File path: flink-core/src/test/java/org/apache/flink/util/NetUtilsTest.java
##########
@@ -61,25 +56,45 @@ public void testParseHostPortAddress() {
@Test
public void testAcceptWithoutTimeout() throws IOException {
// Validates that acceptWithoutTimeout suppresses all
SocketTimeoutExceptions
- ServerSocket serverSocket = mock(ServerSocket.class);
- when(serverSocket.accept())
- .thenAnswer(
- new Answer<Socket>() {
- private int count = 0;
-
- @Override
- public Socket answer(InvocationOnMock
invocationOnMock)
- throws Throwable {
- if (count < 2) {
- count++;
- throw new SocketTimeoutException();
- }
-
- return new Socket();
- }
- });
-
- assertNotNull(NetUtils.acceptWithoutTimeout(serverSocket));
+ Socket expected = new Socket();
+ ServerSocket serverSocket =
+ new ServerSocket() {
+ private int count = 0;
+
+ @Override
+ public Socket accept() throws IOException {
+ if (count < 2) {
+ count++;
+ throw new SocketTimeoutException();
+ }
+
+ return expected;
+ }
+ };
+
+ assertEquals(expected, NetUtils.acceptWithoutTimeout(serverSocket));
+
+ // Validates timeout option precondition
+ serverSocket =
+ new ServerSocket() {
+ @Override
+ public Socket accept() throws IOException {
+ return expected;
+ }
+ };
+
+ // non-zero timeout (throw exception)
+ serverSocket.setSoTimeout(5);
+ try {
+ assertEquals(expected,
NetUtils.acceptWithoutTimeout(serverSocket));
+ fail("Expected IllegalArgumentException due to timeout is set to
non-zero value");
+ } catch (IllegalArgumentException e) {
+ // Pass
+ }
+
+ // zero timeout (don't throw exception)
+ serverSocket.setSoTimeout(0);
+ assertEquals(expected, NetUtils.acceptWithoutTimeout(serverSocket));
Review comment:
As is this check doesn't add value because it will never fail without
the first check in this test failing as well. As a separate test however it
would be useful because it would make it obvious what the issue is.
##########
File path: flink-core/src/test/java/org/apache/flink/util/NetUtilsTest.java
##########
@@ -61,25 +56,45 @@ public void testParseHostPortAddress() {
@Test
public void testAcceptWithoutTimeout() throws IOException {
// Validates that acceptWithoutTimeout suppresses all
SocketTimeoutExceptions
- ServerSocket serverSocket = mock(ServerSocket.class);
- when(serverSocket.accept())
- .thenAnswer(
- new Answer<Socket>() {
- private int count = 0;
-
- @Override
- public Socket answer(InvocationOnMock
invocationOnMock)
- throws Throwable {
- if (count < 2) {
- count++;
- throw new SocketTimeoutException();
- }
-
- return new Socket();
- }
- });
-
- assertNotNull(NetUtils.acceptWithoutTimeout(serverSocket));
+ Socket expected = new Socket();
+ ServerSocket serverSocket =
+ new ServerSocket() {
+ private int count = 0;
+
+ @Override
+ public Socket accept() throws IOException {
+ if (count < 2) {
+ count++;
+ throw new SocketTimeoutException();
+ }
+
+ return expected;
+ }
+ };
+
+ assertEquals(expected, NetUtils.acceptWithoutTimeout(serverSocket));
+
+ // Validates timeout option precondition
+ serverSocket =
+ new ServerSocket() {
+ @Override
+ public Socket accept() throws IOException {
+ return expected;
+ }
+ };
+
+ // non-zero timeout (throw exception)
Review comment:
let's add a separate test for this. It can then be as simple as:
```
@Test(expected = IllegalArgumentException.class)
public void testAcceptWithoutTimeoutRejectsSlotWithSoTimeout() {
try (final ServerSocket serverSocket = new ServerSocket(0)) {
serverSocket.setSoTimeout(5);
NetUtils.acceptWithoutTimeout(serverSocket);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
