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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
