[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015611950
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -185,8 +160,13 @@ public void testRemove() throws Exception {
} catch (ConnectException e) {
//ignore
}
+}
-PowerMock.verifyAll();
+private void removeSetup() {
Review Comment:
Yup, makes a lot of sense. Changed in the next commit :)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015611423
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -185,8 +160,13 @@ public void testRemove() throws Exception {
} catch (ConnectException e) {
//ignore
}
Review Comment:
Nice catch, changed in the subsequent commit.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015610959
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -71,112 +75,83 @@ public void setup() {
Long.toString(DEFAULT_OFFSET_COMMIT_INTERVAL_MS));
WorkerConfig config = new StandaloneConfig(workerProps);
committer = new SourceTaskOffsetCommitter(config, executor,
committers);
-Whitebox.setInternalState(SourceTaskOffsetCommitter.class, "log",
mockLog);
}
@SuppressWarnings("unchecked")
@Test
public void testSchedule() {
-Capture taskWrapper = EasyMock.newCapture();
+ArgumentCaptor taskWrapper =
ArgumentCaptor.forClass(Runnable.class);
-EasyMock.expect(executor.scheduleWithFixedDelay(
-EasyMock.capture(taskWrapper),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
+when(executor.scheduleWithFixedDelay(
+taskWrapper.capture(), eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(TimeUnit.MILLISECONDS))
-).andReturn((ScheduledFuture) commitFuture);
-
-PowerMock.replayAll();
+).thenReturn((ScheduledFuture) commitFuture);
committer.schedule(taskId, task);
-assertTrue(taskWrapper.hasCaptured());
assertNotNull(taskWrapper.getValue());
assertEquals(singletonMap(taskId, commitFuture), committers);
-
-PowerMock.verifyAll();
}
@Test
-public void testClose() throws Exception {
+public void testCloseWithinTimeout() throws Exception {
Review Comment:
Fair point, changed in the next commit.
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -71,112 +75,83 @@ public void setup() {
Long.toString(DEFAULT_OFFSET_COMMIT_INTERVAL_MS));
WorkerConfig config = new StandaloneConfig(workerProps);
committer = new SourceTaskOffsetCommitter(config, executor,
committers);
-Whitebox.setInternalState(SourceTaskOffsetCommitter.class, "log",
mockLog);
}
@SuppressWarnings("unchecked")
@Test
public void testSchedule() {
-Capture taskWrapper = EasyMock.newCapture();
+ArgumentCaptor taskWrapper =
ArgumentCaptor.forClass(Runnable.class);
-EasyMock.expect(executor.scheduleWithFixedDelay(
-EasyMock.capture(taskWrapper),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
+when(executor.scheduleWithFixedDelay(
+taskWrapper.capture(), eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(TimeUnit.MILLISECONDS))
-).andReturn((ScheduledFuture) commitFuture);
-
-PowerMock.replayAll();
+).thenReturn((ScheduledFuture) commitFuture);
committer.schedule(taskId, task);
-assertTrue(taskWrapper.hasCaptured());
assertNotNull(taskWrapper.getValue());
assertEquals(singletonMap(taskId, commitFuture), committers);
-
-PowerMock.verifyAll();
}
@Test
-public void testClose() throws Exception {
+public void testCloseWithinTimeout() throws Exception {
long timeoutMs = 1000;
// Normal termination, where termination times out.
-executor.shutdown();
-PowerMock.expectLastCall();
+when(executor.awaitTermination(timeoutMs,
TimeUnit.MILLISECONDS)).thenReturn(false);
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andReturn(false);
-mockLog.error(EasyMock.anyString());
-PowerMock.expectLastCall();
-PowerMock.replayAll();
+try (LogCaptureAppender logCaptureAppender =
LogCaptureAppender.createAndRegister(SourceTaskOffsetCommitter.class)) {
+committer.close(timeoutMs);
+assertTrue(logCaptureAppender.getEvents().stream().anyMatch(e ->
e.getLevel().equals("ERROR")));
+}
-committer.close(timeoutMs);
+verify(executor).shutdown();
+}
-PowerMock.verifyAll();
-PowerMock.resetAll();
+@Test
+public void testCloseOutsideOfTimeout() throws InterruptedException {
Review Comment:
I agree, I have changed it in the subsequent commit.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015279648
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -71,122 +74,74 @@ public void setup() {
Long.toString(DEFAULT_OFFSET_COMMIT_INTERVAL_MS));
WorkerConfig config = new StandaloneConfig(workerProps);
committer = new SourceTaskOffsetCommitter(config, executor,
committers);
-Whitebox.setInternalState(SourceTaskOffsetCommitter.class, "log",
mockLog);
}
@SuppressWarnings("unchecked")
@Test
public void testSchedule() {
-Capture taskWrapper = EasyMock.newCapture();
+ArgumentCaptor taskWrapper =
ArgumentCaptor.forClass(Runnable.class);
-EasyMock.expect(executor.scheduleWithFixedDelay(
-EasyMock.capture(taskWrapper),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
+when(executor.scheduleWithFixedDelay(
+taskWrapper.capture(), eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(TimeUnit.MILLISECONDS))
-).andReturn((ScheduledFuture) commitFuture);
-
-PowerMock.replayAll();
+).thenReturn((ScheduledFuture) commitFuture);
committer.schedule(taskId, task);
-assertTrue(taskWrapper.hasCaptured());
assertNotNull(taskWrapper.getValue());
assertEquals(singletonMap(taskId, commitFuture), committers);
-
-PowerMock.verifyAll();
}
@Test
public void testClose() throws Exception {
long timeoutMs = 1000;
// Normal termination, where termination times out.
-executor.shutdown();
-PowerMock.expectLastCall();
-
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andReturn(false);
-mockLog.error(EasyMock.anyString());
-PowerMock.expectLastCall();
-PowerMock.replayAll();
+when(executor.awaitTermination(timeoutMs,
TimeUnit.MILLISECONDS)).thenReturn(false);
committer.close(timeoutMs);
-PowerMock.verifyAll();
-PowerMock.resetAll();
-
// Termination interrupted
-executor.shutdown();
-PowerMock.expectLastCall();
-
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andThrow(new InterruptedException());
-PowerMock.replayAll();
+when(executor.awaitTermination(timeoutMs,
TimeUnit.MILLISECONDS)).thenThrow(new InterruptedException());
committer.close(timeoutMs);
-PowerMock.verifyAll();
+verify(executor, times(2)).shutdown();
}
@Test
public void testRemove() throws Exception {
// Try to remove a non-existing task
-PowerMock.replayAll();
-
assertTrue(committers.isEmpty());
committer.remove(taskId);
assertTrue(committers.isEmpty());
-PowerMock.verifyAll();
-PowerMock.resetAll();
-
// Try to remove an existing task
-EasyMock.expect(taskFuture.cancel(eq(false))).andReturn(false);
-EasyMock.expect(taskFuture.isDone()).andReturn(false);
-EasyMock.expect(taskFuture.get()).andReturn(null);
-EasyMock.expect(taskId.connector()).andReturn("MyConnector");
-EasyMock.expect(taskId.task()).andReturn(1);
-PowerMock.replayAll();
+when(taskFuture.cancel(false)).thenReturn(false);
+when(taskFuture.isDone()).thenReturn(false);
+when(taskFuture.get())
+.thenReturn(null)
+.thenThrow(new CancellationException())
+.thenThrow(new InterruptedException());
+when(taskId.connector()).thenReturn("MyConnector");
+when(taskId.task()).thenReturn(1);
committers.put(taskId, taskFuture);
committer.remove(taskId);
assertTrue(committers.isEmpty());
-PowerMock.verifyAll();
-PowerMock.resetAll();
-
// Try to remove a cancelled task
-EasyMock.expect(taskFuture.cancel(eq(false))).andReturn(false);
-EasyMock.expect(taskFuture.isDone()).andReturn(false);
-EasyMock.expect(taskFuture.get()).andThrow(new
CancellationException());
-EasyMock.expect(taskId.connector()).andReturn("MyConnector");
-EasyMock.expect(taskId.task()).andReturn(1);
-mockLog.trace(EasyMock.anyString(), EasyMock.anyObject());
Review Comment:
Amended in the subsequent commit!
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015276765
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -71,122 +74,74 @@ public void setup() {
Long.toString(DEFAULT_OFFSET_COMMIT_INTERVAL_MS));
WorkerConfig config = new StandaloneConfig(workerProps);
committer = new SourceTaskOffsetCommitter(config, executor,
committers);
-Whitebox.setInternalState(SourceTaskOffsetCommitter.class, "log",
mockLog);
}
@SuppressWarnings("unchecked")
@Test
public void testSchedule() {
-Capture taskWrapper = EasyMock.newCapture();
+ArgumentCaptor taskWrapper =
ArgumentCaptor.forClass(Runnable.class);
-EasyMock.expect(executor.scheduleWithFixedDelay(
-EasyMock.capture(taskWrapper),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
+when(executor.scheduleWithFixedDelay(
+taskWrapper.capture(), eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(TimeUnit.MILLISECONDS))
-).andReturn((ScheduledFuture) commitFuture);
-
-PowerMock.replayAll();
+).thenReturn((ScheduledFuture) commitFuture);
committer.schedule(taskId, task);
-assertTrue(taskWrapper.hasCaptured());
assertNotNull(taskWrapper.getValue());
assertEquals(singletonMap(taskId, commitFuture), committers);
-
-PowerMock.verifyAll();
}
@Test
public void testClose() throws Exception {
long timeoutMs = 1000;
// Normal termination, where termination times out.
-executor.shutdown();
-PowerMock.expectLastCall();
-
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andReturn(false);
-mockLog.error(EasyMock.anyString());
-PowerMock.expectLastCall();
-PowerMock.replayAll();
+when(executor.awaitTermination(timeoutMs,
TimeUnit.MILLISECONDS)).thenReturn(false);
committer.close(timeoutMs);
-PowerMock.verifyAll();
-PowerMock.resetAll();
-
// Termination interrupted
-executor.shutdown();
-PowerMock.expectLastCall();
-
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andThrow(new InterruptedException());
-PowerMock.replayAll();
+when(executor.awaitTermination(timeoutMs,
TimeUnit.MILLISECONDS)).thenThrow(new InterruptedException());
committer.close(timeoutMs);
-PowerMock.verifyAll();
+verify(executor, times(2)).shutdown();
}
@Test
public void testRemove() throws Exception {
Review Comment:
I have split this into 4 tests as you suggested. I have a slight preference
for only abstracting the setup of the mocks, but if you would prefer to have as
little duplication as possible I will change it.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015261223
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -71,122 +74,74 @@ public void setup() {
Long.toString(DEFAULT_OFFSET_COMMIT_INTERVAL_MS));
WorkerConfig config = new StandaloneConfig(workerProps);
committer = new SourceTaskOffsetCommitter(config, executor,
committers);
-Whitebox.setInternalState(SourceTaskOffsetCommitter.class, "log",
mockLog);
}
@SuppressWarnings("unchecked")
@Test
public void testSchedule() {
-Capture taskWrapper = EasyMock.newCapture();
+ArgumentCaptor taskWrapper =
ArgumentCaptor.forClass(Runnable.class);
-EasyMock.expect(executor.scheduleWithFixedDelay(
-EasyMock.capture(taskWrapper),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
+when(executor.scheduleWithFixedDelay(
+taskWrapper.capture(), eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(TimeUnit.MILLISECONDS))
-).andReturn((ScheduledFuture) commitFuture);
-
-PowerMock.replayAll();
+).thenReturn((ScheduledFuture) commitFuture);
committer.schedule(taskId, task);
-assertTrue(taskWrapper.hasCaptured());
assertNotNull(taskWrapper.getValue());
assertEquals(singletonMap(taskId, commitFuture), committers);
-
-PowerMock.verifyAll();
}
@Test
public void testClose() throws Exception {
long timeoutMs = 1000;
// Normal termination, where termination times out.
-executor.shutdown();
-PowerMock.expectLastCall();
-
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andReturn(false);
-mockLog.error(EasyMock.anyString());
-PowerMock.expectLastCall();
-PowerMock.replayAll();
+when(executor.awaitTermination(timeoutMs,
TimeUnit.MILLISECONDS)).thenReturn(false);
committer.close(timeoutMs);
Review Comment:
I have opted for splitting this test case into two. Verify should now be
present in both.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12823: KAFKA-14132: Replace PowerMock and EasyMock with Mockito in connect tests
clolov commented on code in PR #12823:
URL: https://github.com/apache/kafka/pull/12823#discussion_r1015259138
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SourceTaskOffsetCommitterTest.java:
##
@@ -71,122 +74,74 @@ public void setup() {
Long.toString(DEFAULT_OFFSET_COMMIT_INTERVAL_MS));
WorkerConfig config = new StandaloneConfig(workerProps);
committer = new SourceTaskOffsetCommitter(config, executor,
committers);
-Whitebox.setInternalState(SourceTaskOffsetCommitter.class, "log",
mockLog);
}
@SuppressWarnings("unchecked")
@Test
public void testSchedule() {
-Capture taskWrapper = EasyMock.newCapture();
+ArgumentCaptor taskWrapper =
ArgumentCaptor.forClass(Runnable.class);
-EasyMock.expect(executor.scheduleWithFixedDelay(
-EasyMock.capture(taskWrapper),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
+when(executor.scheduleWithFixedDelay(
+taskWrapper.capture(), eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(DEFAULT_OFFSET_COMMIT_INTERVAL_MS),
eq(TimeUnit.MILLISECONDS))
-).andReturn((ScheduledFuture) commitFuture);
-
-PowerMock.replayAll();
+).thenReturn((ScheduledFuture) commitFuture);
committer.schedule(taskId, task);
-assertTrue(taskWrapper.hasCaptured());
assertNotNull(taskWrapper.getValue());
assertEquals(singletonMap(taskId, commitFuture), committers);
-
-PowerMock.verifyAll();
}
@Test
public void testClose() throws Exception {
long timeoutMs = 1000;
// Normal termination, where termination times out.
-executor.shutdown();
-PowerMock.expectLastCall();
-
-EasyMock.expect(executor.awaitTermination(eq(timeoutMs),
eq(TimeUnit.MILLISECONDS)))
-.andReturn(false);
-mockLog.error(EasyMock.anyString());
Review Comment:
This is a very fair and clean suggestion. I have added it to the subsequent
commits.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
