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/09 14:46:44 UTC
[GitHub] [flink] afedulov commented on a diff in pull request #19660: [FLINK-27185][connectors] Convert connector modules to assertj
afedulov commented on code in PR #19660:
URL: https://github.com/apache/flink/pull/19660#discussion_r868080704
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/hybrid/HybridSourceTest.java:
##########
@@ -29,9 +29,8 @@
import java.util.List;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
Review Comment:
Usage could be substituted by assertThatThrownBy.
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/hybrid/HybridSourceSplitEnumeratorTest.java:
##########
@@ -158,7 +154,7 @@ public void testHandleSplitRequestAfterSwitchAndReaderReset() {
enumerator.addSplitsBack(Collections.singletonList(splitFromSource0), SUBTASK0);
try {
enumerator.handleSplitRequest(SUBTASK0, "fakehostname");
- Assert.fail("expected exception");
+ fail("expected exception");
Review Comment:
How about assertThatThrownBy?
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/SourceMetricsITCase.java:
##########
@@ -169,45 +168,47 @@ private void assertSourceMetrics(
boolean hasTimestamps) {
List<OperatorMetricGroup> groups =
reporter.findOperatorMetricGroups(jobId, "MetricTestingSource");
- assertThat(groups, hasSize(parallelism));
+ assertThat(groups).hasSize(parallelism);
int subtaskWithMetrics = 0;
for (OperatorMetricGroup group : groups) {
Map<String, Metric> metrics = reporter.getMetricsByGroup(group);
// there are only 2 splits assigned; so two groups will not update metrics
if (group.getIOMetricGroup().getNumRecordsInCounter().getCount() == 0) {
// assert that optional metrics are not initialized when no split assigned
- assertThat(
- metrics.get(MetricNames.CURRENT_EMIT_EVENT_TIME_LAG),
- isGauge(equalTo(InternalSourceReaderMetricGroup.UNDEFINED)));
- assertThat(metrics.get(MetricNames.WATERMARK_LAG), nullValue());
+ assertThat(metrics.get(MetricNames.CURRENT_EMIT_EVENT_TIME_LAG))
+ .satisfies(
+ matching(
+ isGauge(
+ equalTo(
+ InternalSourceReaderMetricGroup
+ .UNDEFINED))));
+ assertThat(metrics.get(MetricNames.WATERMARK_LAG)).isNull();
continue;
}
subtaskWithMetrics++;
// I/O metrics
- assertThat(
- group.getIOMetricGroup().getNumRecordsInCounter(),
- isCounter(equalTo(processedRecordsPerSubtask)));
- assertThat(
- group.getIOMetricGroup().getNumBytesInCounter(),
- isCounter(
- equalTo(
- processedRecordsPerSubtask
- * MockRecordEmitter.RECORD_SIZE_IN_BYTES)));
+ assertThat(group.getIOMetricGroup().getNumRecordsInCounter())
+ .satisfies(matching(isCounter(equalTo(processedRecordsPerSubtask))));
+ assertThat(group.getIOMetricGroup().getNumBytesInCounter())
+ .satisfies(
+ matching(
+ isCounter(
+ equalTo(
+ processedRecordsPerSubtask
+ * MockRecordEmitter
+ .RECORD_SIZE_IN_BYTES))));
// MockRecordEmitter is just incrementing errors every even record
- assertThat(
- metrics.get(MetricNames.NUM_RECORDS_IN_ERRORS),
- isCounter(equalTo(processedRecordsPerSubtask / 2)));
+ assertThat(metrics.get(MetricNames.NUM_RECORDS_IN_ERRORS))
+ .satisfies(matching(isCounter(equalTo(processedRecordsPerSubtask / 2))));
Review Comment:
It seems that those matchers are used extensively and the readability really suffers. Those `isGauge`, `isCounter` methods helper methods have pretty simple logic. I think we should move away from hamcrest as much as possible. This `.satisfies(matching(...)` really diminishes the purpose of introducing matchers for the fluent API.
##########
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcherTest.java:
##########
@@ -230,15 +229,15 @@ public void run() {
while (nextBatch.nextSplit() != null) {
int[] arr;
while ((arr = nextBatch.nextRecordFromSplit()) != null) {
- assertTrue(recordsRead.add(arr[0]));
+ assertThat(recordsRead.add(arr[0])).isTrue();
}
}
}
- assertEquals(numTotalRecords, recordsRead.size());
- assertEquals(0, (int) recordsRead.first());
- assertEquals(numTotalRecords - 1, (int) recordsRead.last());
- assertTrue(wakeupTimes.get() > 0);
+ assertThat(recordsRead).hasSize(numTotalRecords);
+ assertThat((int) recordsRead.first()).isEqualTo(0);
Review Comment:
Why are those (int) cases needed?
--
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