XComp commented on code in PR #23196:
URL: https://github.com/apache/flink/pull/23196#discussion_r1293482931
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/LeaderGatewayRetrieverTest.java:
##########
@@ -60,19 +61,15 @@ public void testGatewayRetrievalFailures() throws Exception
{
settableLeaderRetrievalService.notifyListener(address, leaderId);
// check that the first future has been failed
- try {
- gatewayFuture.get();
-
- fail("The first future should have been failed.");
- } catch (ExecutionException ignored) {
- // that's what we expect
- }
+ assertThatThrownBy(gatewayFuture::get)
+ .as("The first future should have been failed.")
+ .isInstanceOf(ExecutionException.class);
// the second attempt should fail as well
- assertFalse((leaderGatewayRetriever.getNow().isPresent()));
+ assertThat((leaderGatewayRetriever.getNow().isPresent())).isFalse();
Review Comment:
```suggestion
assertThat(leaderGatewayRetriever.getNow()).isNotPresent();
```
nit: the assertj way
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/threadinfo/VertexThreadInfoTrackerTest.java:
##########
@@ -79,7 +80,8 @@
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
/** Tests for the {@link VertexThreadInfoTracker}. */
-public class VertexThreadInfoTrackerTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
```suggestion
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/LeaderGatewayRetrieverTest.java:
##########
@@ -22,26 +22,27 @@
import org.apache.flink.runtime.leaderretrieval.SettableLeaderRetrievalService;
import org.apache.flink.runtime.rpc.RpcGateway;
import org.apache.flink.util.FlinkException;
-import org.apache.flink.util.TestLogger;
+import org.apache.flink.util.TestLoggerExtension;
import org.apache.flink.util.concurrent.FutureUtils;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.mock;
/** Test cases for the {@link LeaderGatewayRetriever}. */
-public class LeaderGatewayRetrieverTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
```suggestion
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/impl/RpcGatewayRetrieverTest.java:
##########
@@ -100,20 +99,20 @@ public void testRpcGatewayRetrieval() throws Exception {
final CompletableFuture<DummyGateway> gatewayFuture =
gatewayRetriever.getFuture();
- assertFalse(gatewayFuture.isDone());
+ assertThat(gatewayFuture.isDone()).isFalse();
settableLeaderRetrievalService.notifyListener(
dummyRpcEndpoint.getAddress(), leaderSessionId);
final DummyGateway dummyGateway =
gatewayFuture.get(TIMEOUT.toMilliseconds(),
TimeUnit.MILLISECONDS);
- assertEquals(dummyRpcEndpoint.getAddress(),
dummyGateway.getAddress());
- assertEquals(
- expectedValue,
- dummyGateway
- .foobar(TIMEOUT)
- .get(TIMEOUT.toMilliseconds(),
TimeUnit.MILLISECONDS));
+
assertThat(dummyGateway.getAddress()).isEqualTo(dummyRpcEndpoint.getAddress());
+ assertThat(
+ dummyGateway
+ .foobar(TIMEOUT)
+ .get(TIMEOUT.toMilliseconds(),
TimeUnit.MILLISECONDS))
+ .isEqualTo(expectedValue);
Review Comment:
```suggestion
FlinkAssertions.assertThatFuture(dummyGateway.foobar(TIMEOUT))
.eventuallySucceeds()
.isEqualTo(expectedValue);
```
nit
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/impl/RpcGatewayRetrieverTest.java:
##########
@@ -28,36 +28,35 @@
import org.apache.flink.runtime.rpc.RpcTimeout;
import org.apache.flink.runtime.rpc.RpcUtils;
import org.apache.flink.runtime.rpc.TestingRpcService;
-import org.apache.flink.util.TestLogger;
+import org.apache.flink.util.TestLoggerExtension;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
import java.util.function.Function;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.assertj.core.api.Assertions.assertThat;
/** Tests for the {@link RpcGatewayRetriever}. */
-public class RpcGatewayRetrieverTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
```suggestion
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/history/HistoryServerUtilsTest.java:
##########
@@ -21,54 +21,55 @@
import org.apache.flink.configuration.Configuration;
import org.apache.flink.configuration.HistoryServerOptions;
import org.apache.flink.configuration.SecurityOptions;
-import org.apache.flink.util.TestLogger;
+import org.apache.flink.util.TestLoggerExtension;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
import javax.annotation.Nonnull;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.Optional;
-import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertThat;
+import static org.assertj.core.api.Assertions.assertThat;
/** Tests for the {@link HistoryServerUtils}. */
-public class HistoryServerUtilsTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
```suggestion
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/WebMonitorEndpointTest.java:
##########
@@ -39,10 +40,11 @@
import java.util.concurrent.TimeUnit;
/** Tests for the {@link WebMonitorEndpoint}. */
-public class WebMonitorEndpointTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
```suggestion
```
We don't need to add this explicitly. The `TestLoggerExtension` is
automatically inject by one of the `org.junit.jupiter.api.extension.Extension`
resource files. tbh I couldn't identify the one that triggers the injection. We
have so many laying around and even removing the ones which I thought would be
relevant still didn't disable the extension :shrug: Maybe, we should create a
ticket to clean that up? :thinking:
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -79,27 +71,33 @@
* <li>Correct ordering of ZooKeeper and state handle operations
* </ul>
*/
-public class ZooKeeperStateHandleStoreTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
```suggestion
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/LeaderGatewayRetrieverTest.java:
##########
@@ -60,19 +61,15 @@ public void testGatewayRetrievalFailures() throws Exception
{
settableLeaderRetrievalService.notifyListener(address, leaderId);
// check that the first future has been failed
- try {
- gatewayFuture.get();
-
- fail("The first future should have been failed.");
- } catch (ExecutionException ignored) {
- // that's what we expect
- }
+ assertThatThrownBy(gatewayFuture::get)
+ .as("The first future should have been failed.")
+ .isInstanceOf(ExecutionException.class);
// the second attempt should fail as well
- assertFalse((leaderGatewayRetriever.getNow().isPresent()));
+ assertThat((leaderGatewayRetriever.getNow().isPresent())).isFalse();
// the third attempt should succeed
- assertEquals(rpcGateway, leaderGatewayRetriever.getNow().get());
+
assertThat(leaderGatewayRetriever.getNow().get()).isEqualTo(rpcGateway);
Review Comment:
```suggestion
assertThat(leaderGatewayRetriever.getNow()).hasValue(rpcGateway);
```
nit: assertj way
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/LeaderGatewayRetrieverTest.java:
##########
@@ -60,19 +61,15 @@ public void testGatewayRetrievalFailures() throws Exception
{
settableLeaderRetrievalService.notifyListener(address, leaderId);
// check that the first future has been failed
- try {
- gatewayFuture.get();
-
- fail("The first future should have been failed.");
- } catch (ExecutionException ignored) {
- // that's what we expect
- }
+ assertThatThrownBy(gatewayFuture::get)
+ .as("The first future should have been failed.")
+ .isInstanceOf(ExecutionException.class);
Review Comment:
```suggestion
FlinkAssertions.assertThatFuture(gatewayFuture)
.as("The first future should have been failed.")
.eventuallyFailsWith(ExecutionException.class);
```
nit: and the comment above is obsolete. It could be removed.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/impl/RpcGatewayRetrieverTest.java:
##########
@@ -123,12 +122,12 @@ public void testRpcGatewayRetrieval() throws Exception {
final DummyGateway dummyGateway2 =
gatewayFuture2.get(TIMEOUT.toMilliseconds(),
TimeUnit.MILLISECONDS);
- assertEquals(dummyRpcEndpoint2.getAddress(),
dummyGateway2.getAddress());
- assertEquals(
- expectedValue2,
- dummyGateway2
- .foobar(TIMEOUT)
- .get(TIMEOUT.toMilliseconds(),
TimeUnit.MILLISECONDS));
+
assertThat(dummyGateway2.getAddress()).isEqualTo(dummyRpcEndpoint2.getAddress());
+ assertThat(
+ dummyGateway2
+ .foobar(TIMEOUT)
+ .get(TIMEOUT.toMilliseconds(),
TimeUnit.MILLISECONDS))
+ .isEqualTo(expectedValue2);
Review Comment:
```suggestion
FlinkAssertions.assertThatFuture(dummyGateway.foobar(TIMEOUT))
.eventuallySucceeds()
.isEqualTo(expectedValue2);
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -110,62 +108,84 @@ public void testAddAndLock() throws Exception {
// Verify
// State handle created
- assertEquals(1, store.getAllAndLock().size());
- assertEquals(state,
store.getAndLock(pathInZooKeeper).retrieveState().getValue());
+ assertThat(store.getAllAndLock().size()).isOne();
+
assertThat(store.getAndLock(pathInZooKeeper).retrieveState().getValue()).isEqualTo(state);
// Path created and is persistent
- Stat stat =
ZOOKEEPER.getClient().checkExists().forPath(pathInZooKeeper);
- assertNotNull(stat);
- assertEquals(0, stat.getEphemeralOwner());
-
- List<String> children =
ZOOKEEPER.getClient().getChildren().forPath(pathInZooKeeper);
+ Stat stat =
+ zooKeeperExtension
+ .getZooKeeperClient(
+
testingFatalErrorHandlerResource.getTestingFatalErrorHandler())
Review Comment:
having a local variable for the client might make the code more readable.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -693,66 +770,79 @@ protected void setStateHandle(
store.addAndLock(
pathInZooKeeper, new
TestingLongStateHandleHelper.LongStateHandle(initialState));
- try {
- store.replace(
- pathInZooKeeper,
- IntegerResourceVersion.valueOf(0),
- new
TestingLongStateHandleHelper.LongStateHandle(replaceState));
- fail("Did not throw expected exception");
- } catch (Throwable t) {
- assertThat(t, IsInstanceOf.instanceOf(expectedException));
- }
+ assertThatThrownBy(
+ () ->
+ store.replace(
+ pathInZooKeeper,
+ IntegerResourceVersion.valueOf(0),
+ new
TestingLongStateHandleHelper.LongStateHandle(
+ replaceState)))
+ .isInstanceOf(expectedException);
Review Comment:
```suggestion
.isInstanceOf(expectedExceptionClass);
```
can we rename the parameter as well?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -962,22 +1068,32 @@ public void testRemove() throws Exception {
store.releaseAndTryRemove(pathInZooKeeper);
// Verify discarded
- assertEquals(0,
ZOOKEEPER.getClient().getChildren().forPath("/").size());
- assertEquals(
- numberOfGlobalDiscardCalls + 1,
- TestingLongStateHandleHelper.getGlobalDiscardCount());
+ assertThat(
+ zooKeeperExtension
+ .getZooKeeperClient(
+ testingFatalErrorHandlerResource
+ .getTestingFatalErrorHandler())
+ .getChildren()
+ .forPath("/")
+ .size())
+ .isZero();
Review Comment:
```suggestion
.forPath("/"))
.isEmpty();
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/LeaderGatewayRetrieverTest.java:
##########
@@ -22,26 +22,27 @@
import org.apache.flink.runtime.leaderretrieval.SettableLeaderRetrievalService;
import org.apache.flink.runtime.rpc.RpcGateway;
import org.apache.flink.util.FlinkException;
-import org.apache.flink.util.TestLogger;
+import org.apache.flink.util.TestLoggerExtension;
import org.apache.flink.util.concurrent.FutureUtils;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.mock;
Review Comment:
Could we add the following class to `org.apache.flink.runtime.rpc` test
package in this PR:
```
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.flink.runtime.rpc;
import java.util.function.Supplier;
/** {@code TestingRpcGateway} is a generic test implementation of {@link
RpcGateway}. */
public class TestingRpcGateway implements RpcGateway {
private final Supplier<String> addressSupplier;
private final Supplier<String> hostnameSupplier;
public TestingRpcGateway(Supplier<String> addressSupplier,
Supplier<String> hostnameSupplier) {
this.addressSupplier = addressSupplier;
this.hostnameSupplier = hostnameSupplier;
}
@Override
public String getAddress() {
return addressSupplier.get();
}
@Override
public String getHostname() {
return hostnameSupplier.get();
}
public static Builder newBuilder() {
return new Builder();
}
/** {@code Builder} for {@code TestingRpcGateway}. */
public static class Builder {
private Supplier<String> addressSupplier = () -> "address";
private Supplier<String> hostnameSupplier = () -> "hostname";
private Builder() {}
public Builder setAddressSupplier(Supplier<String> addressSupplier) {
this.addressSupplier = addressSupplier;
return this;
}
public Builder setHostnameSupplier(Supplier<String>
hostnameSupplier) {
this.hostnameSupplier = hostnameSupplier;
return this;
}
public TestingRpcGateway build() {
return new TestingRpcGateway(addressSupplier, hostnameSupplier);
}
}
}
```
We could use that one to replace the mockito dependency in this test.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/retriever/impl/RpcGatewayRetrieverTest.java:
##########
@@ -100,20 +99,20 @@ public void testRpcGatewayRetrieval() throws Exception {
final CompletableFuture<DummyGateway> gatewayFuture =
gatewayRetriever.getFuture();
- assertFalse(gatewayFuture.isDone());
+ assertThat(gatewayFuture.isDone()).isFalse();
Review Comment:
```suggestion
assertThat(gatewayFuture).isNotDone();
```
nit
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -1260,10 +1444,8 @@ public void
testGetAllHandlesWithMarkedForDeletionEntries() throws Exception {
markNodeForDeletion(client, markedForDeletionNodeName);
- assertThat(
- zkStore.getAllHandles(),
- IsIterableContainingInAnyOrder.containsInAnyOrder(
- notMarkedForDeletionNodeName,
markedForDeletionNodeName));
+ assertThat(zkStore.getAllHandles())
+ .contains(notMarkedForDeletionNodeName,
markedForDeletionNodeName);
Review Comment:
```suggestion
.containsExactlyInAnyOrder(notMarkedForDeletionNodeName,
markedForDeletionNodeName);
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -803,17 +901,19 @@ public void testGetAll() throws Exception {
for
(Tuple2<RetrievableStateHandle<TestingLongStateHandleHelper.LongStateHandle>,
String>
val : store.getAllAndLock()) {
- assertTrue(expected.remove(val.f0.retrieveState().getValue()));
+
assertThat(expected.remove(val.f0.retrieveState().getValue())).isTrue();
}
- assertEquals(0, expected.size());
+ assertThat(expected.size()).isZero();
Review Comment:
```suggestion
assertThat(expected).isEmpty();
```
nit: That's a really nitty remark. Feel free to ignore it.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -110,62 +108,84 @@ public void testAddAndLock() throws Exception {
// Verify
// State handle created
- assertEquals(1, store.getAllAndLock().size());
- assertEquals(state,
store.getAndLock(pathInZooKeeper).retrieveState().getValue());
+ assertThat(store.getAllAndLock().size()).isOne();
+
assertThat(store.getAndLock(pathInZooKeeper).retrieveState().getValue()).isEqualTo(state);
// Path created and is persistent
- Stat stat =
ZOOKEEPER.getClient().checkExists().forPath(pathInZooKeeper);
- assertNotNull(stat);
- assertEquals(0, stat.getEphemeralOwner());
-
- List<String> children =
ZOOKEEPER.getClient().getChildren().forPath(pathInZooKeeper);
+ Stat stat =
+ zooKeeperExtension
+ .getZooKeeperClient(
+
testingFatalErrorHandlerResource.getTestingFatalErrorHandler())
Review Comment:
additionally, having a private method `getClient()` might help
##########
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java:
##########
@@ -1140,13 +1296,13 @@ public void testLockCleanupWhenClientTimesOut() throws
Exception {
Stat stat = client2.checkExists().forPath(path);
// check that our state node still exists
- assertNotNull(stat);
+ assertThat(stat).isNotNull();
Collection<String> children =
client2.getChildren().forPath(zkStore.getRootLockPath(path));
// check that the lock node has been released
- assertEquals(0, children.size());
+ assertThat(children.size()).isZero();
Review Comment:
```suggestion
assertThat(children).isEmpty();
```
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
