afedulov commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r863224050
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java:
##########
@@ -374,12 +366,8 @@ public void testDeserializationWithTypesMismatch() {
new CsvRowDataDeserializationSchema.Builder(rowType,
InternalTypeInfo.of(rowType));
String data = "Test,1,Test";
String errorMessage = "Fail to deserialize at field: f2.";
- try {
- deserialize(deserSchemaBuilder, data);
- fail("expecting exception message:" + errorMessage);
- } catch (Throwable t) {
- assertThat(t, FlinkMatchers.containsMessage(errorMessage));
- }
+ assertThatThrownBy(() -> deserialize(deserSchemaBuilder, data))
+ .satisfies(anyCauseMatches(errorMessage));
Review Comment:
Same.
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDeSerializationSchemaTest.java:
##########
@@ -35,10 +35,8 @@
import java.time.LocalDateTime;
import java.util.function.Consumer;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
Review Comment:
`testDeserializeParseError` could use `assertThatThrownBy().isInstanceOf()`
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/RowCsvInputFormatTest.java:
##########
@@ -38,11 +38,8 @@
import java.sql.Time;
import java.sql.Timestamp;
-import static junit.framework.TestCase.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
Review Comment:
Related method could use `assertThatThrownBy()` instead.
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDeserializationSchemaTest.java:
##########
@@ -43,9 +42,11 @@
import java.util.concurrent.ThreadLocalRandom;
import static
org.apache.flink.formats.utils.DeserializationSchemaMatcher.whenDeserializedWith;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
Review Comment:
The remaining method that uses it could be switched to assertThatThrownBy()
instead.
##########
flink-formats/flink-orc/src/test/java/org/apache/flink/orc/OrcFileSystemFilterTest.java:
##########
@@ -54,7 +54,7 @@ public void testApplyPredicate() {
OrcFilters.Predicate predicate1 =
OrcFilters.toOrcPredicate(equalExpression);
OrcFilters.Predicate predicate2 =
new OrcFilters.Equals("long1", PredicateLeaf.Type.LONG, 10);
- assertTrue(predicate1.toString().equals(predicate2.toString()));
+
assertThat(predicate1.toString().equals(predicate2.toString())).isTrue();
Review Comment:
Why not do `isEqualTo()` directly instead?
##########
flink-formats/flink-orc/src/test/java/org/apache/flink/orc/OrcFileSystemFilterTest.java:
##########
@@ -73,6 +73,6 @@ public void testApplyPredicate() {
OrcFilters.Predicate predicate5 =
OrcFilters.toOrcPredicate(lessExpression);
OrcFilters.Predicate predicate6 =
new OrcFilters.LessThan("long1", PredicateLeaf.Type.LONG, 10);
- assertTrue(predicate5.toString().equals(predicate6.toString()));
+
assertThat(predicate5.toString().equals(predicate6.toString())).isTrue();
Review Comment:
Same.
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDeserializationSchemaTest.java:
##########
@@ -134,7 +135,8 @@ public void testTypeInfoDeserialization() throws Exception {
row.setField(9, map);
row.setField(10, nestedMap);
- assertThat(serializedJson,
whenDeserializedWith(deserializationSchema).equalsTo(row));
+ assertThat(serializedJson)
+
.satisfies(matching(whenDeserializedWith(deserializationSchema).equalsTo(row)));
Review Comment:
I believe the main purpose for the existence of `whenDeserializedWith` was
to make the test readable. The current construct has lost that property and
using this matcher does not seem to bring much benefit. We should either
convert it into a static utility method or consider writing a custom assertion
https://joel-costigliola.github.io/assertj/assertj-core-custom-assertions.html
##########
flink-formats/flink-orc/src/test/java/org/apache/flink/orc/OrcFileSystemFilterTest.java:
##########
@@ -64,7 +64,7 @@ public void testApplyPredicate() {
OrcFilters.Predicate predicate4 =
new OrcFilters.Not(
new OrcFilters.LessThanEquals("long1",
PredicateLeaf.Type.LONG, 10));
- assertTrue(predicate3.toString().equals(predicate4.toString()));
+
assertThat(predicate3.toString().equals(predicate4.toString())).isTrue();
Review Comment:
Same.
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowSerializationSchemaTest.java:
##########
@@ -31,9 +31,9 @@
import java.util.concurrent.ThreadLocalRandom;
import static
org.apache.flink.formats.utils.SerializationSchemaMatcher.whenSerializedWith;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.HamcrestCondition.matching;
Review Comment:
Same as with the JsonRowDeserializationSchemaTest.
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDataSerDeSchemaTest.java:
##########
@@ -73,10 +71,9 @@
import static
org.apache.flink.table.api.DataTypes.TIMESTAMP_WITH_LOCAL_TIME_ZONE;
import static org.apache.flink.table.api.DataTypes.TINYINT;
import static
org.apache.flink.table.types.utils.TypeConversions.fromLogicalToDataType;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-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.assertj.core.api.Assertions.fail;
Review Comment:
Related methods could use assertThatThrownBy() instead.
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDataSerDeSchemaTest.java:
##########
@@ -443,25 +441,25 @@ public void testDeserializationMissingField() throws
Exception {
deserializationSchema.deserialize(serializedJson);
fail("expecting exception message: " + errorMessage);
Review Comment:
I see it is resolved, but the commit still contains the try..catch check.
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java:
##########
@@ -358,12 +354,8 @@ public void testSerializationWithTypesMismatch() {
new CsvRowDataSerializationSchema.Builder(rowType);
RowData rowData = rowData("Test", 1, "Test");
String errorMessage = "Fail to serialize at field: f2.";
- try {
- serialize(serSchemaBuilder, rowData);
- fail("expecting exception message:" + errorMessage);
- } catch (Throwable t) {
- assertThat(t, FlinkMatchers.containsMessage(errorMessage));
- }
+ assertThatThrownBy(() -> serialize(serSchemaBuilder, rowData))
+ .satisfies(anyCauseMatches(errorMessage));
Review Comment:
It looks like this changes the actual check made in this test. The original
was only checking the message of the actual Throwable t, but the new check
tests the whole chain of causes. Not sure how critical it is, but this changes
the semantics of the test.
##########
flink-formats/flink-sequence-file/src/test/java/org/apache/flink/formats/sequencefile/SerializableHadoopConfigurationTest.java:
##########
@@ -55,8 +57,8 @@ public void
customPropertiesSurviveSerializationDeserialization()
final SerializableHadoopConfiguration deserializableConfigUnderTest =
deserializeAndGetConfiguration(serializedConfigUnderTest);
- Assert.assertThat(
- deserializableConfigUnderTest.get(),
hasTheSamePropertiesAs(configuration));
+ assertThat(deserializableConfigUnderTest.get())
+ .satisfies(matching(hasTheSamePropertiesAs(configuration)));
Review Comment:
Same question about the potential custom assertion.
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/debezium/DebeziumJsonSerDeSchemaTest.java:
##########
@@ -97,12 +93,12 @@ public void testPostgresDefaultReplicaIdentify() {
try {
testSerializationDeserialization("debezium-postgres-data-replica-identity.txt",
false);
} catch (Exception e) {
- assertTrue(
- ExceptionUtils.findThrowableWithMessage(
+ assertThat(
Review Comment:
Could we use assertThatThrownBy() + FlinkAssertions instead?
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java:
##########
@@ -61,11 +61,9 @@
import static org.apache.flink.table.data.StringData.fromString;
import static org.apache.flink.table.data.TimestampData.fromInstant;
import static org.apache.flink.table.data.TimestampData.fromLocalDateTime;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-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.assertj.core.api.Assertions.fail;
Review Comment:
Related method could use `assertThatThrownBy()` instead.
--
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]
