You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/13 14:27:25 UTC
[GitHub] [flink] XComp commented on a diff in pull request #19443: [FLINK-25269][rest] Return 503 if rpc endpoint is not started
XComp commented on code in PR #19443:
URL: https://github.com/apache/flink/pull/19443#discussion_r849503979
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/AbstractHandlerTest.java:
##########
@@ -67,16 +67,16 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicReference;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
/** Tests for {@link AbstractHandler}. */
-public class AbstractHandlerTest extends TestLogger {
-
- @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+@ExtendWith({TestLoggerExtension.class})
Review Comment:
```suggestion
@ExtendWith(TestLoggerExtension.class)
```
nit: the curly brackets are obsolete.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/AbstractHandlerTest.java:
##########
@@ -67,16 +67,16 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicReference;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
Review Comment:
There are still two imports referring to junit4
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java:
##########
@@ -703,6 +707,22 @@ public void testDuplicateHandlerRegistrationIsForbidden() throws Exception {
});
}
+ @Test
+ public void testOnUnavailableRpcEndpointReturns503()
+ throws IOException, ExecutionException, InterruptedException {
Review Comment:
```suggestion
throws IOException {
```
```suggestion
throws IOException, ExecutionException, InterruptedException {
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java:
##########
@@ -703,6 +707,22 @@ public void testDuplicateHandlerRegistrationIsForbidden() throws Exception {
});
}
+ @Test
+ public void testOnUnavailableRpcEndpointReturns503()
+ throws IOException, ExecutionException, InterruptedException {
Review Comment:
```suggestion
throws IOException {
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java:
##########
@@ -703,6 +707,22 @@ public void testDuplicateHandlerRegistrationIsForbidden() throws Exception {
});
Review Comment:
Is it only in my setup or do you also have formatting changes that you just didn't commit in this class (e.g. indentation changed in lines 684-687)
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java:
##########
@@ -703,6 +707,22 @@ public void testDuplicateHandlerRegistrationIsForbidden() throws Exception {
});
}
+ @Test
+ public void testOnUnavailableRpcEndpointReturns503()
+ throws IOException, ExecutionException, InterruptedException {
+ CompletableFuture<EmptyResponseBody> response =
+ restClient.sendRequest(
+ serverAddress.getHostName(),
+ serverAddress.getPort(),
+ TestUnavailableHeaders.INSTANCE);
+
+ assertThatThrownBy(() -> response.get())
Review Comment:
```suggestion
assertThatThrownBy(response::get)
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java:
##########
@@ -703,6 +707,22 @@ public void testDuplicateHandlerRegistrationIsForbidden() throws Exception {
});
}
+ @Test
+ public void testOnUnavailableRpcEndpointReturns503()
+ throws IOException, ExecutionException, InterruptedException {
+ CompletableFuture<EmptyResponseBody> response =
+ restClient.sendRequest(
+ serverAddress.getHostName(),
+ serverAddress.getPort(),
+ TestUnavailableHeaders.INSTANCE);
+
+ assertThatThrownBy(() -> response.get())
Review Comment:
```suggestion
assertThatThrownBy(response::get)
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/AbstractHandlerTest.java:
##########
@@ -67,16 +67,16 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicReference;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
/** Tests for {@link AbstractHandler}. */
-public class AbstractHandlerTest extends TestLogger {
-
- @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+@ExtendWith({TestLoggerExtension.class})
Review Comment:
```suggestion
@ExtendWith(TestLoggerExtension.class)
```
nit: the curly brackets are obsolete.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org