1996fanrui commented on code in PR #23196:
URL: https://github.com/apache/flink/pull/23196#discussion_r1293639624
##########
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:
Good catch! Removed them from all classess of this PR!
> 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.
I have removed it on my local, and the `TestLoggerExtension` works.
> 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 🤷 Maybe, we should create a ticket to clean
that up? 🤔
You meant we should clean up the `@ExtendWith(TestLoggerExtension.class)` at
class level for all classes, right?
I have a question about it: could we add the `TestLoggerExtension` to the
`org.junit.jupiter.api.extension.Extension resource` file for all modules?
- If yes, we can remove all `@ExtendWith(TestLoggerExtension.class)` of the
whole flink repo.
- If no, we can remove all `@ExtendWith(TestLoggerExtension.class)` for
modules that added the `TestLoggerExtension` to the
`org.junit.jupiter.api.extension.Extension resource` file.
It should create a ticket whatever yes or no. I created FLINK-32866 to
address 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:
Added the `getZooKeeperClient` method, it's indeed a good ssuggestion, it
can reduce a lot of repeated code.
##########
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:
Good suggestion!
I update the `TestingRpcGateway` constructor from public to private, it
should be created by Builder.
##########
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:
Good catch! Removed them from all classess of this PR!
> 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.
I have removed it on my local, and the `TestLoggerExtension` works.
> 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 🤷 Maybe, we should create a ticket to clean
that up? 🤔
You meant we should clean up the `@ExtendWith(TestLoggerExtension.class)` at
class level for all classes, right?
I have a question about it: could we add the `TestLoggerExtension` to the
`org.junit.jupiter.api.extension.Extension resource` file for all modules?
- If yes, we can remove all `@ExtendWith(TestLoggerExtension.class)` of the
whole flink repo.
- If no, we can remove all `@ExtendWith(TestLoggerExtension.class)` for
modules that added the `TestLoggerExtension` to the
`org.junit.jupiter.api.extension.Extension resource` file.
It should create a ticket whatever yes or no. I created FLINK-32866 to
address it.
--
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
