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/04/22 07:22:20 UTC
[GitHub] [flink] alpreu opened a new pull request, #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
alpreu opened a new pull request, #19552:
URL: https://github.com/apache/flink/pull/19552
This PR fixes and grandfathers #19425
--
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
[GitHub] [flink] afedulov commented on pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
afedulov commented on PR #19552:
URL: https://github.com/apache/flink/pull/19552#issuecomment-1115468523
Please temporarily split the second commit (connectors) into three roughly equal once to ease the review process a bit.
--
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
[GitHub] [flink] MartijnVisser merged pull request #19552: [FLINK-27185][connectors][formats] Convert format modules to assertj
Posted by GitBox <gi...@apache.org>.
MartijnVisser merged PR #19552:
URL: https://github.com/apache/flink/pull/19552
--
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
[GitHub] [flink] afedulov commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
afedulov commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r863602581
##########
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:
Just search where it is used: `testDeserializeParseError()`. Since the line did not change, only the import, it is not possible to comment in the precise location during the review.
--
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
[GitHub] [flink] alpreu commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
alpreu commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r857542559
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcherManagerTest.java:
##########
@@ -100,7 +93,7 @@ private final void testExceptionPropagation(
fetcher.checkErrors();
fail("expected exception");
} catch (Exception e) {
- assertSame(testingException, e.getCause().getCause());
+ assertThat(e.getCause().getCause()).isSameAs(testingException);
Review Comment:
How would you keep the `finally` statement to close the fetcher in this case?
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r857602633
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/KafkaShortRetentionTestBase.java:
##########
@@ -286,7 +286,7 @@ public void runFailOnAutoOffsetResetNoneEager() throws Exception {
fail("should fail with an exception");
} catch (IllegalArgumentException e) {
// expected
- assertTrue(e.getMessage().contains("none"));
+ assertThat(e.getMessage()).contains("none");
Review Comment:
yes, I think you are 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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] afedulov commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [flink] alpreu commented on pull request #19552: [FLINK-27185][connectors][formats] Convert format modules to assertj
Posted by GitBox <gi...@apache.org>.
alpreu commented on PR #19552:
URL: https://github.com/apache/flink/pull/19552#issuecomment-1124603260
> @alpreu looks good apart from one improvement that seem to be missing [#19552 (comment)](https://github.com/apache/flink/pull/19552#discussion_r870601643)
I must have missed that one somehow, I fixed it now
--
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
[GitHub] [flink] afedulov commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert format modules to assertj
Posted by GitBox <gi...@apache.org>.
afedulov commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r870601643
##########
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:
@alpreu this comment is marked as resolved, but the code still contains the try/fail/catch setup.
--
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
[GitHub] [flink] afedulov commented on pull request #19552: [FLINK-27185][connectors][formats] Convert format modules to assertj
Posted by GitBox <gi...@apache.org>.
afedulov commented on PR #19552:
URL: https://github.com/apache/flink/pull/19552#issuecomment-1124136224
@alpreu looks good apart from one improvement that seem to be missing
https://github.com/apache/flink/pull/19552#discussion_r870601643
--
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
[GitHub] [flink] alpreu commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
alpreu commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r863505977
##########
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:
I had another look at it and to me it looks like the original also checks the whole chain. The matcher is calling `findThrowable` which iterates through the chain and checks if the predicate (containsMessage) is correct
--
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
[GitHub] [flink] alpreu commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert format modules to assertj
Posted by GitBox <gi...@apache.org>.
alpreu commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r866808922
##########
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:
As discussed offline, we decided to keep it as is because converting the Matchers into AssertJ Assertion classes introduces a lot of boilerplate and feels very clunky
--
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
[GitHub] [flink] afedulov commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert format modules to assertj
Posted by GitBox <gi...@apache.org>.
afedulov commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r870601643
##########
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:
@alpreu I see that this comment is resolved, but the code still contains the try/fail/catch setup.
--
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
[GitHub] [flink] flinkbot commented on pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19552:
URL: https://github.com/apache/flink/pull/19552#issuecomment-1106103895
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "16349ae9b8c9293dbfbb6034abd58709145565d9",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "16349ae9b8c9293dbfbb6034abd58709145565d9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 16349ae9b8c9293dbfbb6034abd58709145565d9 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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
[GitHub] [flink] alpreu commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
alpreu commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r863507040
##########
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:
Which method are you referring to in this comment and the ones below? There are no linenumbers assigned
--
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
[GitHub] [flink] afedulov commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
afedulov commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r863600886
##########
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:
You are right, I missed that.
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r856240226
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcherManagerTest.java:
##########
@@ -100,7 +93,7 @@ private final void testExceptionPropagation(
fetcher.checkErrors();
fail("expected exception");
} catch (Exception e) {
- assertSame(testingException, e.getCause().getCause());
+ assertThat(e.getCause().getCause()).isSameAs(testingException);
Review Comment:
Shouldn't it be replaced with `assertThatThrownBy(() -> ... )` ?
##########
flink-connectors/flink-connector-cassandra/src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraSinkBaseTest.java:
##########
@@ -159,9 +156,9 @@ public void testThrowErrorOnSnapshot() throws Exception {
try {
testHarness.snapshot(123L, 123L);
- Assert.fail();
+ fail("unknown failure");
} catch (Exception e) {
- Assert.assertTrue(e.getCause() instanceof IOException);
+ assertThat(e.getCause()).isInstanceOf(IOException.class);
}
Review Comment:
Could it be replaced with
```java
assertThatThrownBy(() -> testHarness.snapshot(123L, 123L))
.hasCauseInstanceOf(IOException.class);
```
?
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/internal/connection/SimpleJdbcConnectionProviderTest.java:
##########
@@ -140,19 +130,17 @@ public void testInvalidDriverUrl() throws Exception {
provider.getOrEstablishConnection();
fail("expect exception");
} catch (SQLException ex) {
- assertThat(
Review Comment:
Probably `assertThatThrownBy` could be used like
```java
assertThatThrownBy(provider::getOrEstablishConnection)
.isInstanceOf(SQLException.class)
.hasMessageContaining("No suitable driver found for " + FakeDBUtils.TEST_DB_INVALID_URL);
```
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/utils/JdbcTypeUtilTest.java:
##########
@@ -25,23 +25,23 @@
import java.sql.Types;
import static org.apache.flink.connector.jdbc.utils.JdbcTypeUtil.logicalTypeToSqlType;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
/** Testing the type conversions from Flink to SQL types. */
public class JdbcTypeUtilTest {
@Test
public void testTypeConversions() {
- assertEquals(Types.INTEGER, logicalTypeToSqlType(LogicalTypeRoot.INTEGER));
+ assertThat(logicalTypeToSqlType(LogicalTypeRoot.INTEGER)).isEqualTo(Types.INTEGER);
testUnsupportedType(LogicalTypeRoot.RAW);
testUnsupportedType(LogicalTypeRoot.MAP);
}
private static void testUnsupportedType(LogicalTypeRoot logicalTypeRoot) {
try {
logicalTypeToSqlType(logicalTypeRoot);
- fail();
+ fail("unknown failure");
} catch (IllegalArgumentException ignored) {
}
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/KafkaShortRetentionTestBase.java:
##########
@@ -286,7 +286,7 @@ public void runFailOnAutoOffsetResetNoneEager() throws Exception {
fail("should fail with an exception");
} catch (IllegalArgumentException e) {
// expected
- assertTrue(e.getMessage().contains("none"));
+ assertThat(e.getMessage()).contains("none");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaConnectorOptionsUtilTest.java:
##########
@@ -118,14 +120,15 @@ public void testInvalidKeyFormatPrefixProjection() {
try {
createKeyFormatProjection(config, dataType);
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java:
##########
@@ -196,11 +194,11 @@ public void testSerializeDeserializeCustomizedProperties() throws Exception {
try {
testFieldDeserialization(
TIME(precision), "12:12:12.45", LocalTime.parse("12:12:12"), deserConfig, ";");
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java:
##########
@@ -362,7 +357,7 @@ public void testSerializationWithTypesMismatch() {
serialize(serSchemaBuilder, rowData);
fail("expecting exception message:" + errorMessage);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java:
##########
@@ -378,7 +373,7 @@ public void testDeserializationWithTypesMismatch() {
deserialize(deserSchemaBuilder, data);
fail("expecting exception message:" + errorMessage);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/debezium/DebeziumJsonFormatFactoryTest.java:
##########
@@ -140,12 +140,12 @@ public void testSchemaIncludeOption() {
// should fail
sinkMock.valueFormat.createRuntimeEncoder(
new SinkRuntimeProviderContext(false), PHYSICAL_DATA_TYPE);
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcOutputFormatTest.java:
##########
@@ -146,7 +144,7 @@ public void testInvalidURL() {
outputFormat.open(0, 1);
fail("Expected exception is not thrown.");
} catch (Exception e) {
- assertTrue(findThrowable(e, IllegalStateException.class).isPresent());
+ assertThat(findThrowable(e, IllegalStateException.class)).isPresent();
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcOutputFormatTest.java:
##########
@@ -183,7 +181,7 @@ public void testIncompatibleTypes() {
outputFormat.close();
fail("Expected exception is not thrown.");
} catch (Exception e) {
- assertTrue(findThrowable(e, ClassCastException.class).isPresent());
+ assertThat(findThrowable(e, ClassCastException.class)).isPresent();
}
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcOutputFormatTest.java:
##########
@@ -263,8 +261,8 @@ public void testExceptionOnClose() {
fail("Expected exception is not thrown.");
} catch (Exception e) {
- assertTrue(findThrowable(e, RuntimeException.class).isPresent());
- assertTrue(findThrowableWithMessage(e, expectedMsg).isPresent());
+ assertThat(findThrowable(e, RuntimeException.class)).isPresent();
+ assertThat(findThrowableWithMessage(e, expectedMsg)).isPresent();
}
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/factories/HiveCatalogFactoryTest.java:
##########
@@ -134,7 +132,7 @@ public void testCreateHiveCatalogWithIllegalHadoopConfDir() throws IOException {
options,
null,
Thread.currentThread().getContextClassLoader());
- Assert.fail();
+ fail("unknown failure");
} catch (ValidationException e) {
}
Review Comment:
Probably could be used `assertThatThrownBy` like
```java
assertThatThrownBy(() -> FactoryUtil.createCatalog(
catalogName,
options,
null,
Thread.currentThread().getContextClassLoader()))
.isInstanceOf(ValidationException.class);
```
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcOutputFormatTest.java:
##########
@@ -115,8 +113,8 @@ public void testInvalidDriver() {
outputFormat.open(0, 1);
fail("Expected exception is not thrown.");
} catch (Exception e) {
- assertTrue(findThrowable(e, IOException.class).isPresent());
- assertTrue(findThrowableWithMessage(e, expectedMsg).isPresent());
+ assertThat(findThrowable(e, IOException.class)).isPresent();
+ assertThat(findThrowableWithMessage(e, expectedMsg)).isPresent();
}
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaConnectorOptionsUtilTest.java:
##########
@@ -92,17 +93,18 @@ public void testInvalidKeyFormatFieldProjection() {
try {
createKeyFormatProjection(config, dataType);
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDataSerDeSchemaTest.java:
##########
@@ -562,18 +560,18 @@ public void testSerializationMapNullKey() throws Exception {
try {
// throw exception when mapNullKey Mode is fail
serializationSchema1.serialize(rowData);
- Assert.fail("expecting exception message: " + errorMessage1);
+ fail("expecting exception message: " + errorMessage1);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaProducerITCase.java:
##########
@@ -193,7 +189,7 @@ public void testFlinkKafkaProducerFailBeforeNotify() throws Exception {
try {
testHarness.processElement(44, 4);
testHarness.snapshot(2, 5);
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaConnectorOptionsUtilTest.java:
##########
@@ -71,14 +71,15 @@ public void testMissingKeyFormatProjection() {
try {
createKeyFormatProjection(config, dataType);
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcOutputFormatTest.java:
##########
@@ -220,7 +218,7 @@ public void testExceptionOnInvalidType() {
outputFormat.close();
fail("Expected exception is not thrown.");
} catch (Exception e) {
- assertTrue(findThrowable(e, ClassCastException.class).isPresent());
+ assertThat(findThrowable(e, ClassCastException.class)).isPresent();
}
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
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);
} catch (Throwable t) {
- assertEquals(errorMessage, t.getMessage());
+ assertThat(t).hasMessage(errorMessage);
}
// ignore on parse error
deserializationSchema =
new JsonRowDataDeserializationSchema(
schema, InternalTypeInfo.of(schema), false, true, TimestampFormat.ISO_8601);
actual = convertToExternal(deserializationSchema.deserialize(serializedJson), dataType);
- assertEquals(expected, actual);
+ assertThat(actual).isEqualTo(expected);
errorMessage =
"JSON format doesn't support failOnMissingField and ignoreParseErrors are both enabled.";
try {
// failOnMissingField and ignoreParseErrors both enabled
new JsonRowDataDeserializationSchema(
schema, InternalTypeInfo.of(schema), true, true, TimestampFormat.ISO_8601);
- Assert.fail("expecting exception message: " + errorMessage);
+ fail("expecting exception message: " + errorMessage);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDataSerDeSchemaTest.java:
##########
@@ -703,7 +701,7 @@ private void testParseErrors(TestSpec spec) throws Exception {
failingSchema.deserialize(spec.json.getBytes());
fail("expecting exception " + spec.errorMessage);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaConnectorOptionsUtilTest.java:
##########
@@ -140,14 +143,15 @@ public void testInvalidValueFormatProjection() {
try {
createValueFormatProjection(config, dataType);
- fail();
+ fail("unknown failure");
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
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:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDataSerDeSchemaTest.java:
##########
@@ -649,7 +647,7 @@ public void testSerializationWithTypesMismatch() {
serializationSchema.serialize(genericRowData);
fail("expecting exception message: " + errorMessage);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
##########
flink-formats/flink-json/src/test/java/org/apache/flink/formats/json/JsonRowDataSerDeSchemaTest.java:
##########
@@ -665,7 +663,7 @@ public void testDeserializationWithTypesMismatch() {
deserializationSchema.deserialize(json.getBytes());
fail("expecting exception message: " + errorMessage);
Review Comment:
Probably `try ... catch` could be replaced with `assertThatThrownBy`
--
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
[GitHub] [flink] alpreu commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
alpreu commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r857573227
##########
flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/KafkaShortRetentionTestBase.java:
##########
@@ -286,7 +286,7 @@ public void runFailOnAutoOffsetResetNoneEager() throws Exception {
fail("should fail with an exception");
} catch (IllegalArgumentException e) {
// expected
- assertTrue(e.getMessage().contains("none"));
+ assertThat(e.getMessage()).contains("none");
Review Comment:
Looks like this method (and the one above) are not even used anymore. Maybe we should remove it, what do you think?
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #19552: [FLINK-27185][connectors][formats] Convert formats and connectors modules to assertj
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19552:
URL: https://github.com/apache/flink/pull/19552#discussion_r857601787
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcherManagerTest.java:
##########
@@ -100,7 +93,7 @@ private final void testExceptionPropagation(
fetcher.checkErrors();
fail("expected exception");
} catch (Exception e) {
- assertSame(testingException, e.getCause().getCause());
+ assertThat(e.getCause().getCause()).isSameAs(testingException);
Review Comment:
Ahh... `finally`, yes you are right, sorry
--
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