ferenc-csaky commented on code in PR #23301:
URL: https://github.com/apache/flink/pull/23301#discussion_r1321298136
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rpc/RpcEndpointTest.java:
##########
@@ -91,35 +85,31 @@ public void testSelfGateway() throws Exception {
* by the RpcEndpoint.
*/
@Test
- public void testWrongSelfGateway() {
- assertThrows(
- RuntimeException.class,
- () -> {
- int expectedValue = 1337;
- BaseEndpoint baseEndpoint = new BaseEndpoint(rpcService,
expectedValue);
-
- try {
- baseEndpoint.start();
-
- DifferentGateway differentGateway =
+ void testWrongSelfGateway() {
+ int expectedValue = 1337;
+ BaseEndpoint baseEndpoint = new BaseEndpoint(rpcService,
expectedValue);
+ assertThatThrownBy(
+ () -> {
+ try {
+ baseEndpoint.start();
baseEndpoint.getSelfGateway(DifferentGateway.class);
-
- fail(
- "Expected to fail with a RuntimeException
since we requested the wrong gateway type.");
- } finally {
- RpcUtils.terminateRpcEndpoint(baseEndpoint);
-
- baseEndpoint.validateResourceClosed();
- }
- });
+ } finally {
+ RpcUtils.terminateRpcEndpoint(baseEndpoint);
+
+ baseEndpoint.validateResourceClosed();
+ }
+ })
+ .withFailMessage(
+ "Expected to fail with a RuntimeException since we
requested the wrong gateway type.")
+ .isInstanceOf(RuntimeException.class);
Review Comment:
IMO it makes the test more readable to only include the code that throws the
exception inside the `assertThatThrownBy` call:
```java
try {
baseEndpoint.start();
assertThatThrownBy(() ->
baseEndpoint.getSelfGateway(DifferentGateway.class))
.withFailMessage(
"Expected to fail with a RuntimeException since
we requested the wrong gateway type.")
.isInstanceOf(RuntimeException.class);
} finally {
RpcUtils.terminateRpcEndpoint(baseEndpoint);
baseEndpoint.validateResourceClosed();
}
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rpc/FencedRpcEndpointTest.java:
##########
@@ -88,20 +84,20 @@ public void testFencing() throws Exception {
FencedTestingGateway.class)
.get(timeout.toMilliseconds(),
TimeUnit.MILLISECONDS);
- assertEquals(
- value,
- properFencedGateway
- .foobar(timeout)
- .get(timeout.toMilliseconds(),
TimeUnit.MILLISECONDS));
+ assertThat(
+ properFencedGateway
+ .foobar(timeout)
+ .get(timeout.toMilliseconds(),
TimeUnit.MILLISECONDS))
+ .isEqualTo(value);
try {
wronglyFencedGateway
.foobar(timeout)
.get(timeout.toMilliseconds(), TimeUnit.MILLISECONDS);
fail("This should fail since we have the wrong fencing
token.");
} catch (ExecutionException e) {
- assertTrue(
- ExceptionUtils.stripExecutionException(e) instanceof
FencingTokenException);
+ assertThat(ExceptionUtils.stripExecutionException(e))
Review Comment:
This try/catch can be removed completely:
```java
assertThatThrownBy(
() ->
wronglyFencedGateway
.foobar(timeout)
.get(timeout.toMilliseconds(),
TimeUnit.MILLISECONDS))
.hasRootCauseInstanceOf(FencingTokenException.class);
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rpc/RpcSSLAuthITCase.java:
##########
@@ -79,55 +77,62 @@ public void testConnectFailure() throws Exception {
sslConfig2.setString(SecurityOptions.SSL_INTERNAL_TRUSTSTORE_PASSWORD,
"password");
sslConfig2.setString(SecurityOptions.SSL_ALGORITHMS,
"TLS_RSA_WITH_AES_128_CBC_SHA");
- RpcService rpcService1 = null;
- RpcService rpcService2 = null;
-
- try {
- // to test whether the test is still good:
- // - create actorSystem2 with sslConfig1 (same as actorSystem1)
and see that both can
- // connect
- // - set 'require-mutual-authentication = off' in the
ConfigUtils ssl config section
- rpcService1 =
- RpcSystem.load()
- .localServiceBuilder(sslConfig1)
- .withBindAddress("localhost")
- .withBindPort(0)
- .createAndStart();
- rpcService2 =
- RpcSystem.load()
- .localServiceBuilder(sslConfig2)
- .withBindAddress("localhost")
- .withBindPort(0)
- .createAndStart();
-
- TestEndpoint endpoint = new TestEndpoint(rpcService1);
- endpoint.start();
-
- CompletableFuture<TestGateway> future =
- rpcService2.connect(endpoint.getAddress(),
TestGateway.class);
- TestGateway gateway = future.get(10000000, TimeUnit.SECONDS);
-
- CompletableFuture<String> fooFuture = gateway.foo();
- fooFuture.get();
-
- fail("should never complete normally");
- } catch (ExecutionException e) {
- // that is what we want
- assertTrue(e.getCause() instanceof RpcConnectionException);
- } finally {
- final CompletableFuture<Void> rpcTerminationFuture1 =
- rpcService1 != null
- ? rpcService1.closeAsync()
- : CompletableFuture.completedFuture(null);
-
- final CompletableFuture<Void> rpcTerminationFuture2 =
- rpcService2 != null
- ? rpcService2.closeAsync()
- : CompletableFuture.completedFuture(null);
-
- FutureUtils.waitForAll(Arrays.asList(rpcTerminationFuture1,
rpcTerminationFuture2))
- .get();
- }
+ assertThatThrownBy(
Review Comment:
IMO it makes the test more readable to only include the code that throws the
exception inside the `assertThatThrownBy` call:
```java
assertThatThrownBy(
() -> {
TestGateway gateway = future.get(10000000,
TimeUnit.SECONDS);
CompletableFuture<String> fooFuture =
gateway.foo();
fooFuture.get();
})
.withFailMessage("should never complete normally")
.isInstanceOf(ExecutionException.class)
.hasCauseInstanceOf(RpcConnectionException.class);
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rpc/FencedRpcEndpointTest.java:
##########
@@ -136,16 +132,15 @@ public void testUnfencedRemoteGateway() throws Exception {
.get(timeout.toMilliseconds(), TimeUnit.MILLISECONDS);
fail("This should have failed because we have an unfenced
gateway.");
} catch (ExecutionException e) {
- assertTrue(
- ExceptionUtils.stripExecutionException(e) instanceof
RpcRuntimeException);
+ assertThat(ExceptionUtils.stripExecutionException(e))
+ .isInstanceOf(RpcRuntimeException.class);
Review Comment:
This try/catch can be removed:
```java
assertThatThrownBy(
() ->
unfencedGateway
.foobar(timeout)
.get(timeout.toMilliseconds(),
TimeUnit.MILLISECONDS))
.hasRootCauseInstanceOf(RpcRuntimeException.class);
```
--
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]
