You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/02 23:32:37 UTC
[GitHub] [flink] afedulov commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org