1996fanrui commented on code in PR #23233:
URL: https://github.com/apache/flink/pull/23233#discussion_r1304565575
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/InPlaceMutableHashTableTest.java:
##########
@@ -466,7 +463,7 @@ public void testWithLengthChangingReduceFunction() throws
Exception {
// Check results
- assertEquals(expectedOutput.size(), actualOutput.size());
+ assertThat(actualOutput.size()).isEqualTo(expectedOutput.size());
Review Comment:
```suggestion
assertThat(actualOutput).hasSameSizeAs(expectedOutput);
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/drivers/ReduceCombineDriverTest.java:
##########
@@ -33,12 +33,14 @@
import org.apache.flink.types.StringValue;
import org.apache.flink.util.MutableObjectIterator;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
import java.util.Collections;
import java.util.List;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
Review Comment:
```
public void testReduceDriverMutable()
public void testImmutableEmpty()
```
These 2 public can be removed.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/AbstractOuterJoinTaskTest.java:
##########
@@ -379,16 +380,18 @@ public void run() {
taskRunner.join(60000);
- assertFalse("Task thread did not finish within 60 seconds",
taskRunner.isAlive());
+ assertThat(taskRunner.isAlive())
+ .withFailMessage("Task thread did not finish within 60
seconds")
+ .isFalse();
final Throwable taskError = error.get();
if (taskError != null) {
fail("Error in task while canceling:\n" +
Throwables.getStackTraceAsString(taskError));
Review Comment:
How about assertThat(taskError).withFailMessage("xxx").isNull()?
This class has 3 similar code, LeftOuterJoinTaskTest has 2, and
RightOuterJoinTaskTest has 2, please update them if make sense to you.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/HashTableITCase.java:
##########
@@ -101,8 +99,8 @@ public void setup() {
this.ioManager = new IOManagerAsync();
}
- @After
- public void tearDown() throws Exception {
+ @AfterEach
+ void tearDown() throws Exception {
// shut down I/O manager and Memory Manager and verify the correct
shutdown
this.ioManager.close();
if (!this.memManager.verifyEmpty()) {
Review Comment:
assertThat(this.memManager.verifyEmpty()).withFailMessage("xxx").isTrue();
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/CompactingHashTableTest.java:
##########
@@ -117,14 +118,14 @@ public void testHashTableGrowthWithInsert() {
MutableObjectIterator<Tuple2<Long, String>> iter =
table.getEntryIterator();
Tuple2<Long, String> next;
while ((next = iter.next()) != null) {
- assertNotNull(next.f0);
- assertNotNull(next.f1);
- assertEquals(next.f0.longValue(), Long.parseLong(next.f1));
+ assertThat(next.f0).isNotNull();
+ assertThat(next.f1).isNotNull();
+ assertThat(next.f0).isEqualTo(Long.parseLong(next.f1));
Review Comment:
```suggestion
assertThat(next.f1).isNotNull();
assertThat(next.f0).isNotNull().isEqualTo(Long.parseLong(next.f1));
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/drivers/GroupReduceDriverTest.java:
##########
@@ -235,22 +237,16 @@ public void
testAllReduceDriverIncorrectlyAccumulatingMutable() {
Object[] res = result.getList().toArray();
Object[] expected =
DriverTestData.createReduceMutableDataGroupedResult().toArray();
- try {
- DriverTestData.compareTupleArrays(expected, res);
- Assert.fail(
- "Accumulationg mutable objects is expected to result
in incorrect values.");
- } catch (AssertionError e) {
- // expected
- }
+ DriverTestData.compareTupleArrays(expected, res);
Review Comment:
Why it doesn't throw `AssertionError` after your change? Is it as expected?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/InPlaceMutableHashTableTest.java:
##########
@@ -352,7 +349,7 @@ public void testWithIntPair() throws Exception {
// Check results
- assertEquals(expectedOutput.size(), actualOutput.size());
+ assertThat(actualOutput.size()).isEqualTo(expectedOutput.size());
Review Comment:
```suggestion
assertThat(actualOutput).hasSameSizeAs(expectedOutput);
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/AbstractOuterJoinTaskTest.java:
##########
@@ -300,15 +300,15 @@ public void testMergeOuterJoinTask() throws Exception {
final int expCnt = calculateExpectedCount(keyCnt1, valCnt1, keyCnt2,
valCnt2);
- Assert.assertTrue(
- "Result set size was " + this.outList.size() + ". Expected was
" + expCnt,
- this.outList.size() == expCnt);
+ assertThat(this.outList)
+ .withFailMessage("Result set size was %d. Expected was %d",
outList.size(), expCnt)
+ .hasSize(expCnt);
this.outList.clear();
}
- @Test(expected = ExpectedTestException.class)
- public void testFailingOuterJoinTask() throws Exception {
+ @TestTemplate
+ void testFailingOuterJoinTask() throws Exception {
Review Comment:
```suggestion
void testFailingOuterJoinTask() {
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/HashTableITCase.java:
##########
@@ -735,10 +728,9 @@ public void testSparseProbeSpilling() throws IOException,
MemoryAllocationExcept
numRecordsInJoinResult++;
}
}
- Assert.assertEquals(
- "Wrong number of records in join result.",
- expectedNumResults,
- numRecordsInJoinResult);
+ assertThat(numRecordsInJoinResult)
+ .withFailMessage("Wrong number of records in join result.")
+ .isEqualTo(expectedNumResults);
Review Comment:
I cannot comment at `public void testSparseProbeSpillingWithOuterJoin()`
line, so comment here.
the public can be removed.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/CrossTaskTest.java:
##########
@@ -127,17 +134,17 @@ public void testFailingBlockCrossTask() {
try {
testDriver(testTask, MockFailingCrossStub.class);
- Assert.fail("Exception not forwarded.");
+ fail("Exception not forwarded.");
} catch (ExpectedTestException etex) {
// good!
} catch (Exception e) {
e.printStackTrace();
- Assert.fail("Test failed due to an exception.");
+ fail("Test failed due to an exception.");
}
Review Comment:
assertThrown
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/InPlaceMutableHashTableTest.java:
##########
@@ -151,14 +148,14 @@ public void testHashTableGrowthWithInsert() {
MutableObjectIterator<Tuple2<Long, String>> iter =
table.getEntryIterator();
Tuple2<Long, String> next;
while ((next = iter.next()) != null) {
- assertNotNull(next.f0);
- assertNotNull(next.f1);
- assertEquals(next.f0.longValue(), Long.parseLong(next.f1));
+ assertThat(next.f0).isNotNull();
+ assertThat(next.f1).isNotNull();
+ assertThat(next.f0).isEqualTo(Long.parseLong(next.f1));
Review Comment:
```suggestion
assertThat(next.f1).isNotNull();
assertThat(next.f0).isNotNull().isEqualTo(Long.parseLong(next.f1));
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/InPlaceMutableHashTableTest.java:
##########
@@ -209,14 +206,14 @@ public void testHashTableGrowthWithInsertOrReplace() {
MutableObjectIterator<Tuple2<Long, String>> iter =
table.getEntryIterator();
Tuple2<Long, String> next;
while ((next = iter.next()) != null) {
- assertNotNull(next.f0);
- assertNotNull(next.f1);
- assertEquals(next.f0.longValue(), Long.parseLong(next.f1));
+ assertThat(next.f0).isNotNull();
+ assertThat(next.f1).isNotNull();
+ assertThat(next.f0).isEqualTo(Long.parseLong(next.f1));
Review Comment:
```suggestion
assertThat(next.f1).isNotNull();
assertThat(next.f0).isNotNull().isEqualTo(Long.parseLong(next.f1));
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/InPlaceMutableHashTableTest.java:
##########
@@ -560,7 +557,7 @@ private static String getLongString(int length) {
}
@Test
- public void testOutOfMemory() {
+ void testOutOfMemory() {
Review Comment:
The `EOFException` can be replaced with `assertThrownXXX`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/hash/CompactingHashTableTest.java:
##########
@@ -176,14 +177,14 @@ public void testHashTableGrowthWithInsertOrReplace() {
MutableObjectIterator<Tuple2<Long, String>> iter =
table.getEntryIterator();
Tuple2<Long, String> next;
while ((next = iter.next()) != null) {
- assertNotNull(next.f0);
- assertNotNull(next.f1);
- assertEquals(next.f0.longValue(), Long.parseLong(next.f1));
+ assertThat(next.f0).isNotNull();
+ assertThat(next.f1).isNotNull();
+ assertThat(next.f0).isEqualTo(Long.parseLong(next.f1));
Review Comment:
Same to the last comment
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/drivers/DriverTestData.java:
##########
@@ -100,7 +100,9 @@ public static List<Tuple2<StringValue, IntValue>>
createReduceMutableDataGrouped
public static final void compareTupleArrays(Object[] expected, Object[]
found) {
if (expected.length != found.length) {
- Assert.assertEquals("Length of result is wrong", expected.length,
found.length);
+ assertThat(found.length)
+ .withFailMessage("Length of result is wrong")
+ .isEqualTo(expected.length);
Review Comment:
```suggestion
assertThat(found)
.withFailMessage("Length of result is wrong")
.hasSameSizeAs(expected);
```
BTW, the `if` and the `assert` is odd.
- If the length are not same, assert them to equal
- If the length are same, don't assert
That means we can assert directly, the if can be removed, right?
--
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
