divijvaidya commented on code in PR #12735:
URL: https://github.com/apache/kafka/pull/12735#discussion_r996391100
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/ErrorHandlingTaskTest.java:
##########
@@ -224,85 +225,64 @@ public void tearDown() {
if (metrics != null) {
metrics.stop();
}
+
}
@Test
public void testSinkTasksCloseErrorReporters() throws Exception {
- ErrorReporter reporter = EasyMock.mock(ErrorReporter.class);
+ ErrorReporter reporter = mock(ErrorReporter.class);
RetryWithToleranceOperator retryWithToleranceOperator = operator();
retryWithToleranceOperator.reporters(singletonList(reporter));
createSinkTask(initialState, retryWithToleranceOperator);
- expectInitializeTask();
- reporter.close();
Review Comment:
we are missing verification call for this.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/ErrorHandlingTaskTest.java:
##########
@@ -349,13 +330,15 @@ public void testErrorHandlingInSinkTasks() throws
Exception {
// one record completely failed (converter issues), and thus was
skipped
assertErrorHandlingMetricValue("total-records-skipped", 1.0);
- PowerMock.verifyAll();
}
private RetryWithToleranceOperator operator() {
- return new RetryWithToleranceOperator(OPERATOR_RETRY_TIMEOUT_MILLIS,
OPERATOR_RETRY_MAX_DELAY_MILLIS, OPERATOR_TOLERANCE_TYPE, SYSTEM,
errorHandlingMetrics);
+ return new RetryWithToleranceOperator(OPERATOR_RETRY_TIMEOUT_MILLIS,
+ OPERATOR_RETRY_MAX_DELAY_MILLIS, OPERATOR_TOLERANCE_TYPE,
+ SYSTEM, errorHandlingMetrics);
}
+// @Ignore
Review Comment:
Can we remove this now if the test is running fine?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/ErrorHandlingTaskTest.java:
##########
@@ -139,7 +141,8 @@ public class ErrorHandlingTaskTest {
@SuppressWarnings("unused")
@Mock
private SourceTask sourceTask;
- private Capture<WorkerSinkTaskContext> sinkTaskContext =
EasyMock.newCapture();
+ @Captor
+ private ArgumentCaptor<WorkerSinkTaskContext> sinkTaskContext;
Review Comment:
I think that this argument capture isn't being used anywhere. We never call
`sinkTaskContext.getValue()`. Perhaps we can remove this?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/ErrorHandlingTaskTest.java:
##########
@@ -224,85 +225,64 @@ public void tearDown() {
if (metrics != null) {
metrics.stop();
}
+
Review Comment:
nit
extra line
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/ErrorHandlingTaskTest.java:
##########
@@ -494,72 +494,27 @@ private void assertErrorHandlingMetricValue(String name,
double expected) {
assertEquals(expected, measured, 0.001d);
}
- private void expectInitializeTask() {
- consumer.subscribe(EasyMock.eq(singletonList(TOPIC)),
EasyMock.capture(rebalanceListener));
- PowerMock.expectLastCall();
-
- sinkTask.initialize(EasyMock.capture(sinkTaskContext));
- PowerMock.expectLastCall();
- sinkTask.start(TASK_PROPS);
- PowerMock.expectLastCall();
- }
+ private void expectClose() throws IOException {
Review Comment:
suggestion:
perhaps rename to `verifyClose()`
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/ErrorHandlingTaskTest.java:
##########
@@ -224,85 +225,64 @@ public void tearDown() {
if (metrics != null) {
metrics.stop();
}
+
}
@Test
public void testSinkTasksCloseErrorReporters() throws Exception {
- ErrorReporter reporter = EasyMock.mock(ErrorReporter.class);
+ ErrorReporter reporter = mock(ErrorReporter.class);
RetryWithToleranceOperator retryWithToleranceOperator = operator();
retryWithToleranceOperator.reporters(singletonList(reporter));
createSinkTask(initialState, retryWithToleranceOperator);
- expectInitializeTask();
Review Comment:
We are missing some verification for calls which were earlier in this
function.
I would suggest to create a method called `verifyInitializeTask()` which
would verify the following invocations:
1. `verify(sinkTask).start(TASK_PROPS);`
2. `verify(sinkTask).initialize(any(WorkerSinkTaskContext.class));`
3. `verify(consumer).subscribe(singletonList(TOPIC), rebalanceListener);`
You can use the method to verify initialize calls in all tests that were
earlier using `expectInitializeTask()` earlier.
--
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]
