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/12 08:11:45 UTC

[GitHub] [kafka] yashmayya opened a new pull request, #12509: KAFKA-14133: Replace EasyMock with Mockito in WorkerCoordinatorTest and RootResourceTest

yashmayya opened a new pull request, #12509:
URL: https://github.com/apache/kafka/pull/12509

   - This PR replaces `EasyMock` with `Mockito` in the remaining Connect tests from https://issues.apache.org/jira/browse/KAFKA-14133
   - The Connect tests which still use `EasyMock` are ones which also use `PowerMock` and listed in https://issues.apache.org/jira/browse/KAFKA-14132


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

[GitHub] [kafka] dplavcic commented on a diff in pull request #12509: KAFKA-14133: Replace EasyMock with Mockito in WorkerCoordinatorTest and RootResourceTest

Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12509:
URL: https://github.com/apache/kafka/pull/12509#discussion_r944801602


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/RootResourceTest.java:
##########
@@ -20,21 +20,20 @@
 import org.apache.kafka.common.utils.AppInfoParser;
 import org.apache.kafka.connect.runtime.Herder;
 import org.apache.kafka.connect.runtime.rest.entities.ServerInfo;
-import org.easymock.EasyMock;
-import org.easymock.EasyMockRunner;
-import org.easymock.EasyMockSupport;
-import org.easymock.Mock;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
 
 import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
-@RunWith(EasyMockRunner.class)
-public class RootResourceTest extends EasyMockSupport {
+@RunWith(MockitoJUnitRunner.class)

Review Comment:
   To align this test with others we are currenclt migrating, can you please use `MockitoJUnitRunner.StrictStubs.class` instead of the `MockitoJUnitRunner.class`?
   
   Mockito docs explaning what `StrictStubs` does:
   - https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/junit/MockitoJUnitRunner.StrictStubs.html
   - https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/quality/Strictness.html#STRICT_STUBS



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

[GitHub] [kafka] yashmayya commented on a diff in pull request #12509: KAFKA-14133: Replace EasyMock with Mockito in WorkerCoordinatorTest and RootResourceTest

Posted by GitBox <gi...@apache.org>.
yashmayya commented on code in PR #12509:
URL: https://github.com/apache/kafka/pull/12509#discussion_r949456092


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorTest.java:
##########
@@ -432,30 +422,28 @@ public void testLeaderPerformAssignment1() {
         assertEquals(Collections.emptyList(), memberAssignment.connectors());
         assertEquals(Collections.singletonList(taskId1x0), memberAssignment.tasks());
 
-        PowerMock.verifyAll();
+        verify(configStorage).snapshot();
     }
 
     @Test
     public void testLeaderPerformAssignment2() {
         // Since all the protocol responses are mocked, the other tests validate doSync runs, but don't validate its
         // output. So we test it directly here.
 
-        EasyMock.expect(configStorage.snapshot()).andReturn(configState2);
-
-        PowerMock.replayAll();
+        when(configStorage.snapshot()).thenReturn(configState2);
 
         // Prime the current configuration state
         coordinator.metadata();
 
-        // Mark everyone as in sync with configState1
+        // Mark everyone as in sync with configState2
         List<JoinGroupResponseData.JoinGroupResponseMember> responseMembers = new ArrayList<>();
         responseMembers.add(new JoinGroupResponseData.JoinGroupResponseMember()
                 .setMemberId("leader")
-                .setMetadata(ConnectProtocol.serializeMetadata(new ConnectProtocol.WorkerState(LEADER_URL, 1L)).array())
+                .setMetadata(ConnectProtocol.serializeMetadata(new ConnectProtocol.WorkerState(LEADER_URL, 2L)).array())

Review Comment:
   Yep, makes sense. I thought about making these changes originally, but refrained from doing so with the thought that it might be a little noisy and detract from the focus of the PR. Thanks for the feedback, I've made the requisite changes!



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

[GitHub] [kafka] mimaison merged pull request #12509: KAFKA-14133: Replace EasyMock with Mockito in WorkerCoordinatorTest and RootResourceTest

Posted by GitBox <gi...@apache.org>.
mimaison merged PR #12509:
URL: https://github.com/apache/kafka/pull/12509


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

[GitHub] [kafka] yashmayya commented on pull request #12509: KAFKA-14133: Replace EasyMock with Mockito in WorkerCoordinatorTest and RootResourceTest

Posted by GitBox <gi...@apache.org>.
yashmayya commented on PR #12509:
URL: https://github.com/apache/kafka/pull/12509#issuecomment-1219063626

   @mimaison @C0urante could one of you please take a look?


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

[GitHub] [kafka] mimaison commented on a diff in pull request #12509: KAFKA-14133: Replace EasyMock with Mockito in WorkerCoordinatorTest and RootResourceTest

Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12509:
URL: https://github.com/apache/kafka/pull/12509#discussion_r948858351


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorTest.java:
##########
@@ -432,30 +422,28 @@ public void testLeaderPerformAssignment1() {
         assertEquals(Collections.emptyList(), memberAssignment.connectors());
         assertEquals(Collections.singletonList(taskId1x0), memberAssignment.tasks());
 
-        PowerMock.verifyAll();
+        verify(configStorage).snapshot();
     }
 
     @Test
     public void testLeaderPerformAssignment2() {
         // Since all the protocol responses are mocked, the other tests validate doSync runs, but don't validate its
         // output. So we test it directly here.
 
-        EasyMock.expect(configStorage.snapshot()).andReturn(configState2);
-
-        PowerMock.replayAll();
+        when(configStorage.snapshot()).thenReturn(configState2);
 
         // Prime the current configuration state
         coordinator.metadata();
 
-        // Mark everyone as in sync with configState1
+        // Mark everyone as in sync with configState2
         List<JoinGroupResponseData.JoinGroupResponseMember> responseMembers = new ArrayList<>();
         responseMembers.add(new JoinGroupResponseData.JoinGroupResponseMember()
                 .setMemberId("leader")
-                .setMetadata(ConnectProtocol.serializeMetadata(new ConnectProtocol.WorkerState(LEADER_URL, 1L)).array())
+                .setMetadata(ConnectProtocol.serializeMetadata(new ConnectProtocol.WorkerState(LEADER_URL, 2L)).array())

Review Comment:
   Should we use `configState2.offset()` instead of the literal? 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