[GitHub] [kafka] yashmayya commented on a diff in pull request #12472: KAFKA-14134: Replace EasyMock with Mockito for WorkerConnectorTest

[GitBox <gi...@apache.org>]

yashmayya commented on code in PR #12472:
URL: https://github.com/apache/kafka/pull/12472#discussion_r941154092


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -672,6 +452,27 @@ protected void assertInitializedMetric(WorkerConnector workerConnector, String e
         assertEquals(VERSION, version);
     }
 
+    @SuppressWarnings("unchecked")
+    private Callback<TargetState> mockCallback() {
+        return mock(Callback.class);
+    }
+
+    private void verifyCleanInitialize() {
+        verify(connector).version();
+        if (connector instanceof SourceConnector) {
+            verify(offsetStore).start();
+            verify(connector).initialize(any(SourceConnectorContext.class));
+        } else {
+            verify(connector).initialize(any(SinkConnectorContext.class));
+        }
+    }
+
+    private void verifyCleanShutdown() {
+        verify(ctx).close();
+        verify(offsetStorageReader).close();

Review Comment:
   Good catch, thanks! I just translated the tests as is and hadn't noticed that we were passing non-null offset storage reader and offset backing stores instances even for sink connectors in these tests (differing in behavior with how the actual `Worker` instantiates `WorkerConnector`s).



##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -120,43 +105,21 @@ public void testInitializeFailure() {
         workerConnector.doShutdown();
         assertStoppedMetric(workerConnector);
 
-        verifyAll();
+        verifyCleanInitialize();
+        verify(listener).onFailure(CONNECTOR, exception);
+        verify(listener).onShutdown(CONNECTOR);

Review Comment:
   The one for shutdown makes sense but I think with startup there are three different cases - 
   
   i) No call to `connector.start` or `listener.onStartup` (failure in initialization OR started in the paused state)
   ii) Calls to both `connector.start` and `listener.onStartup` (successful start)
   iii) Call to only `connector.start` (connector started in the paused state and then resumed OR failure on startup)
   
   I think it might be more readable to keep these verify calls in the test methods as is rather than trying to force fit them into (in)appropriately named methods, WDYT?



-- 
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