You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/08/02 18:55:09 UTC
[GitHub] [kafka] C0urante commented on a diff in pull request #12472: KAFKA-14134: Replace EasyMock with Mockito for WorkerConnectorTest
C0urante commented on code in PR #12472:
URL: https://github.com/apache/kafka/pull/12472#discussion_r935923454
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -120,43 +101,26 @@ public void testInitializeFailure() {
workerConnector.doShutdown();
assertStoppedMetric(workerConnector);
- verifyAll();
+ verify(connector).version();
+ verify(offsetStore).start();
+ verify(connector).initialize(any(SourceConnectorContext.class));
+ verify(listener).onFailure(CONNECTOR, exception);
+ verify(listener).onShutdown(CONNECTOR);
+ verify(ctx).close();
+ verify(offsetStorageReader).close();
+ verify(offsetStore).stop();
}
@Test
public void testFailureIsFinalState() {
RuntimeException exception = new RuntimeException();
connector = sinkConnector;
- connector.version();
- expectLastCall().andReturn(VERSION);
-
- connector.initialize(EasyMock.notNull(SinkConnectorContext.class));
- expectLastCall().andThrow(exception);
-
- listener.onFailure(CONNECTOR, exception);
- expectLastCall();
-
- // expect no call to onStartup() after failure
-
- listener.onShutdown(CONNECTOR);
- expectLastCall();
-
- ctx.close();
- expectLastCall();
-
- offsetStorageReader.close();
- expectLastCall();
-
- offsetStore.stop();
- expectLastCall();
-
- Callback<TargetState> onStateChange = createStrictMock(Callback.class);
- onStateChange.onCompletion(EasyMock.anyObject(Exception.class), EasyMock.isNull());
- expectLastCall();
-
- replayAll();
+ when(connector.version()).thenReturn(VERSION);
+ doThrow(exception).when(connector).initialize(any());
+ @SuppressWarnings("unchecked")
+ Callback<TargetState> onStateChange = mock(Callback.class);
Review Comment:
What do you think about introducing a utility method so that creating callbacks like these can be done without having to add the `@SuppressWarnings` annotation?
I'm thinking something like this:
```java
@SuppressWarnings("unchecked")
private Callback<TargetState> mockCallback() {
return mock(Callback.class);
}
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -60,15 +64,15 @@ public class WorkerConnectorTest extends EasyMockSupport {
public ConnectorConfig connectorConfig;
public MockConnectMetrics metrics;
- @Mock Plugins plugins;
- @Mock SourceConnector sourceConnector;
- @Mock SinkConnector sinkConnector;
- @Mock Connector connector;
- @Mock CloseableConnectorContext ctx;
- @Mock ConnectorStatus.Listener listener;
- @Mock CloseableOffsetStorageReader offsetStorageReader;
- @Mock ConnectorOffsetBackingStore offsetStore;
- @Mock ClassLoader classLoader;
+ private final Plugins plugins = mock(Plugins.class);
Review Comment:
Can we use the `@Mock` annotation for these?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -120,43 +101,26 @@ public void testInitializeFailure() {
workerConnector.doShutdown();
assertStoppedMetric(workerConnector);
- verifyAll();
+ verify(connector).version();
+ verify(offsetStore).start();
+ verify(connector).initialize(any(SourceConnectorContext.class));
+ verify(listener).onFailure(CONNECTOR, exception);
+ verify(listener).onShutdown(CONNECTOR);
+ verify(ctx).close();
+ verify(offsetStorageReader).close();
+ verify(offsetStore).stop();
Review Comment:
I know that this complexity isn't introduced by these changes, and is only translated from the existing EasyMock/PowerMock tests, but it'd be nice if we could try to clean up this logic while we're in the neighborhood.
Would you mind taking a stab at grouping some of these together into reusable methods, like `verifyCleanStart`, `verifyCleanShutdown`, etc.? I tried to do that in https://github.com/apache/kafka/pull/12409 if you're looking for inspiration, but you don't have to match that exact style if you have other ideas/preferences.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -275,52 +197,31 @@ public void testStartupAndPause() {
workerConnector.doShutdown();
assertStoppedMetric(workerConnector);
- verifyAll();
+ verify(connector).version();
+ verify(connector).initialize(any(SinkConnectorContext.class));
+ verify(connector).start(CONFIG);
+ verify(listener).onStartup(CONNECTOR);
+ verify(connector).stop();
+ verify(listener).onPause(CONNECTOR);
+ verify(listener).onShutdown(CONNECTOR);
+ verify(ctx).close();
+ verify(offsetStorageReader).close();
+ verify(offsetStore).stop();
+
+ InOrder inOrder = inOrder(onStateChange);
+ inOrder.verify(onStateChange).onCompletion(isNull(), eq(TargetState.STARTED));
+ inOrder.verify(onStateChange).onCompletion(isNull(), eq(TargetState.PAUSED));
+ verifyNoMoreInteractions(onStateChange);
Review Comment:
Very nice 👍
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -60,15 +64,15 @@ public class WorkerConnectorTest extends EasyMockSupport {
public ConnectorConfig connectorConfig;
public MockConnectMetrics metrics;
- @Mock Plugins plugins;
- @Mock SourceConnector sourceConnector;
- @Mock SinkConnector sinkConnector;
- @Mock Connector connector;
- @Mock CloseableConnectorContext ctx;
- @Mock ConnectorStatus.Listener listener;
- @Mock CloseableOffsetStorageReader offsetStorageReader;
- @Mock ConnectorOffsetBackingStore offsetStore;
- @Mock ClassLoader classLoader;
+ private final Plugins plugins = mock(Plugins.class);
+ private final SourceConnector sourceConnector = mock(SourceConnector.class);
+ private final SinkConnector sinkConnector = mock(SinkConnector.class);
+ private final CloseableConnectorContext ctx = mock(CloseableConnectorContext.class);
+ private final ConnectorStatus.Listener listener = mock(ConnectorStatus.Listener.class);
+ private final CloseableOffsetStorageReader offsetStorageReader = mock(CloseableOffsetStorageReader.class);
+ private final ConnectorOffsetBackingStore offsetStore = mock(ConnectorOffsetBackingStore.class);
+ private final ClassLoader classLoader = mock(ClassLoader.class);
+ private Connector connector;
Review Comment:
I think it helps with readability a bit. We always need a `Connector` for our tests. The reason we can't use the `@Mock` annotation for it is just that we always use either the (mocked) `SourceConnector` or `SinkConnector`.
Also worth noting that if https://github.com/apache/kafka/pull/12307 lands, a class-level `Connector` would be easier to work with.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org