XComp commented on code in PR #19916:
URL: https://github.com/apache/flink/pull/19916#discussion_r962898568
##########
flink-connectors/flink-connector-cassandra/src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraTupleWriteAheadSinkTest.java:
##########
@@ -39,17 +40,18 @@
import java.util.concurrent.atomic.AtomicReference;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anyString;
-import static org.powermock.api.mockito.PowerMockito.doAnswer;
-import static org.powermock.api.mockito.PowerMockito.mock;
-import static org.powermock.api.mockito.PowerMockito.when;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
/** Tests for the {@link CassandraTupleWriteAheadSink}. */
-public class CassandraTupleWriteAheadSinkTest {
+class CassandraTupleWriteAheadSinkTest {
- @Test(timeout = 20000)
- public void testAckLoopExitOnException() throws Exception {
+ @Test
+ @Timeout(20)
Review Comment:
```suggestion
@Timeout(value = 20_000, unit = TimeUnit.MILLISECONDS)
```
I'd suggest making the unit more explicit here to improve readability.
Alternatively, we could move the value into a static class member where the
name includes the timeunit. But using the more elaborate annotation signatures
here is good enough, I guess.
##########
flink-connectors/flink-connector-cassandra/src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraSinkBaseTest.java:
##########
@@ -46,15 +47,15 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.fail;
import static org.mockito.Mockito.mock;
-import static org.powermock.api.mockito.PowerMockito.when;
+import static org.mockito.Mockito.when;
/** Tests for the {@link CassandraSinkBase}. */
-public class CassandraSinkBaseTest {
+class CassandraSinkBaseTest {
- private static final long DEFAULT_TEST_TIMEOUT = 5000;
+ private static final long DEFAULT_TEST_TIMEOUT = 5;
Review Comment:
```suggestion
private static final long DEFAULT_TEST_TIMEOUT_IN_SECS = 5;
```
May we also update the name if we change the value already. To improve
readability.
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSinkTest.java:
##########
@@ -122,15 +123,15 @@ protected void verifyResultsWhenReScaling(
}
Collections.sort(sink.values);
- Assert.assertArrayEquals(list.toArray(), sink.values.toArray());
+ assertThat(sink.values.toArray()).containsExactly(list.toArray());
}
@Test
Review Comment:
I know it's a bit out of scope but could we also move the `@Test` annotation
underneath the JavaDoc here? ...probably in a separate hotfix commit
##########
flink-connectors/flink-connector-cassandra/src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraConnectorITCase.java:
##########
@@ -101,7 +102,9 @@
@SuppressWarnings("serial")
// NoHostAvailableException is raised by Cassandra client under load while
connecting to the cluster
@RetryOnException(times = 3, exception = NoHostAvailableException.class)
-public class CassandraConnectorITCase
+@Testcontainers
+@ExtendWith(RetryExtension.class)
Review Comment:
The `RetryExtension` does not seem to work as expected. I created
[FLINK-29198](https://issues.apache.org/jira/browse/FLINK-29198) as a follow-up
because it's out-of-scope for that ticket.
##########
flink-connectors/flink-connector-cassandra/src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraSinkBaseTest.java:
##########
@@ -46,15 +47,15 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.fail;
import static org.mockito.Mockito.mock;
-import static org.powermock.api.mockito.PowerMockito.when;
+import static org.mockito.Mockito.when;
Review Comment:
What's the reasoning behind switching from `PowerMockito` to `Mockito`?
(disclaimer: I didn't do any research on the differences between the two)
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSinkTest.java:
##########
@@ -122,15 +123,15 @@ protected void verifyResultsWhenReScaling(
}
Collections.sort(sink.values);
- Assert.assertArrayEquals(list.toArray(), sink.values.toArray());
+ assertThat(sink.values.toArray()).containsExactly(list.toArray());
Review Comment:
```suggestion
assertThat(sink.values).containsExactly(list);
```
Can't we go for a simpler solution here? :thinking:
--
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
