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 2021/06/07 16:11:00 UTC

[GitHub] [kafka] wycccccc opened a new pull request #10835: KAFKA12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

wycccccc opened a new pull request #10835:
URL: https://github.com/apache/kafka/pull/10835


   Development of EasyMock and PowerMock has stagnated while Mockito continues to be actively developed. With the new Java cadence, it's a problem to depend on libraries that do bytecode generation and are not actively maintained. In addition, Mockito is also easier to [use.KAFKA-7438](https://issues.apache.org/jira/browse/KAFKA-7438?jql=project%20%3D%20KAFKA%20AND%20status%20in%20(Open%2C%20Reopened)%20AND%20assignee%20in%20(EMPTY)%20AND%20text%20~%20%22mockito%22)
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] cadonna commented on pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#issuecomment-858498491


   Test failures are unrelated and known to be flaky:
   ```
   Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
   Build / JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
   ```
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#issuecomment-856794295


   @cadonna The build changes look good.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] wycccccc commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
wycccccc commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646847971



##########
File path: build.gradle
##########
@@ -1484,6 +1484,7 @@ project(':storage') {
     testImplementation project(':clients').sourceSets.test.output
     testImplementation libs.junitJupiter
     testImplementation libs.mockitoCore
+    testImplementation libs.mockitoInline // supports mocking static methods, final classes, etc.

Review comment:
       sor,my mis.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] cadonna merged pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
cadonna merged pull request #10835:
URL: https://github.com/apache/kafka/pull/10835


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] wycccccc commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
wycccccc commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r648148202



##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/NamedCacheMetricsTest.java
##########
@@ -49,37 +41,28 @@
     private static final String HIT_RATIO_MIN_DESCRIPTION = "The minimum cache hit ratio";
     private static final String HIT_RATIO_MAX_DESCRIPTION = "The maximum cache hit ratio";
 
-    private final StreamsMetricsImpl streamsMetrics = createMock(StreamsMetricsImpl.class);
-    private final Sensor expectedSensor = mock(Sensor.class);
+    private Sensor expectedSensor = Mockito.mock(Sensor.class);
     private final Map<String, String> tagMap = mkMap(mkEntry("key", "value"));
 
     @Test
     public void shouldGetHitRatioSensorWithBuiltInMetricsVersionCurrent() {
         final String hitRatio = "hit-ratio";
-        mockStatic(StreamsMetricsImpl.class);
-        expect(streamsMetrics.version()).andStubReturn(Version.LATEST);
-        expect(streamsMetrics.cacheLevelSensor(THREAD_ID, TASK_ID, STORE_NAME, hitRatio, RecordingLevel.DEBUG))
-            .andReturn(expectedSensor);
-        expect(streamsMetrics.cacheLevelTagMap(THREAD_ID, TASK_ID, STORE_NAME)).andReturn(tagMap);
+        final StreamsMetricsImpl streamsMetrics = Mockito.mock(StreamsMetricsImpl.class);
+        Mockito.when(streamsMetrics.cacheLevelSensor(THREAD_ID, TASK_ID, STORE_NAME, hitRatio, RecordingLevel.DEBUG)).thenReturn(expectedSensor);
+        Mockito.when(streamsMetrics.cacheLevelTagMap(THREAD_ID, TASK_ID, STORE_NAME)).thenReturn(tagMap);
         StreamsMetricsImpl.addAvgAndMinAndMaxToSensor(
-            expectedSensor,
-            StreamsMetricsImpl.CACHE_LEVEL_GROUP,
-            tagMap,
-            hitRatio,
-            HIT_RATIO_AVG_DESCRIPTION,
-            HIT_RATIO_MIN_DESCRIPTION,
-            HIT_RATIO_MAX_DESCRIPTION);
-        replay(streamsMetrics);
-        replay(StreamsMetricsImpl.class);
+                expectedSensor,
+                StreamsMetricsImpl.CACHE_LEVEL_GROUP,
+                tagMap,
+                hitRatio,
+                HIT_RATIO_AVG_DESCRIPTION,
+                HIT_RATIO_MIN_DESCRIPTION,
+                HIT_RATIO_MAX_DESCRIPTION);

Review comment:
       Thank you for the review. @ijuma  @chia7712 @cadonna 




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646825247



##########
File path: build.gradle
##########
@@ -1484,6 +1484,7 @@ project(':storage') {
     testImplementation project(':clients').sourceSets.test.output
     testImplementation libs.junitJupiter
     testImplementation libs.mockitoCore
+    testImplementation libs.mockitoInline // supports mocking static methods, final classes, etc.

Review comment:
       Why have we made this change?

##########
File path: build.gradle
##########
@@ -1710,6 +1711,8 @@ project(':streams') {
     testImplementation libs.powermockEasymock
     testImplementation libs.bcpkix
     testImplementation libs.hamcrest
+    testImplementation libs.mockitoCore

Review comment:
       We probably don't need this, right?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] wycccccc commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
wycccccc commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646780507



##########
File path: build.gradle
##########
@@ -1710,6 +1710,7 @@ project(':streams') {
     testImplementation libs.powermockEasymock

Review comment:
       There are other dependencies that have not been modified, so this cannot be removed .




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] wycccccc commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
wycccccc commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646802528



##########
File path: streams/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
##########
@@ -0,0 +1,2 @@
+mock-maker-inline

Review comment:
       Thanks suggestion, I have modified.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] wycccccc commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
wycccccc commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646848688



##########
File path: build.gradle
##########
@@ -1710,6 +1711,8 @@ project(':streams') {
     testImplementation libs.powermockEasymock
     testImplementation libs.bcpkix
     testImplementation libs.hamcrest
+    testImplementation libs.mockitoCore

Review comment:
       Yes,It's extra.I have removed.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] chia7712 commented on a change in pull request #10835: KAFKA12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646742679



##########
File path: streams/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
##########
@@ -0,0 +1,2 @@
+mock-maker-inline

Review comment:
       Why we need this file?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] wycccccc commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
wycccccc commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r648145898



##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/NamedCacheMetricsTest.java
##########
@@ -49,37 +41,28 @@
     private static final String HIT_RATIO_MIN_DESCRIPTION = "The minimum cache hit ratio";
     private static final String HIT_RATIO_MAX_DESCRIPTION = "The maximum cache hit ratio";
 
-    private final StreamsMetricsImpl streamsMetrics = createMock(StreamsMetricsImpl.class);
-    private final Sensor expectedSensor = mock(Sensor.class);
+    private Sensor expectedSensor = Mockito.mock(Sensor.class);

Review comment:
       OK,I have changed.Thanks your suggestion.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] chia7712 commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r647503080



##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/NamedCacheMetricsTest.java
##########
@@ -49,37 +41,28 @@
     private static final String HIT_RATIO_MIN_DESCRIPTION = "The minimum cache hit ratio";
     private static final String HIT_RATIO_MAX_DESCRIPTION = "The maximum cache hit ratio";
 
-    private final StreamsMetricsImpl streamsMetrics = createMock(StreamsMetricsImpl.class);
-    private final Sensor expectedSensor = mock(Sensor.class);
+    private Sensor expectedSensor = Mockito.mock(Sensor.class);

Review comment:
       How about adding ‘final’ back?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10835: KAFKA12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646771843



##########
File path: streams/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
##########
@@ -0,0 +1,2 @@
+mock-maker-inline

Review comment:
       I don't think we need this although if we use mocking of statics, we would need to have a dependency on mockito-inline vs mockito-core (like we do in `tools`).




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] cadonna commented on a change in pull request #10835: KAFKA-12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r647259690



##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/NamedCacheMetricsTest.java
##########
@@ -49,37 +41,28 @@
     private static final String HIT_RATIO_MIN_DESCRIPTION = "The minimum cache hit ratio";
     private static final String HIT_RATIO_MAX_DESCRIPTION = "The maximum cache hit ratio";
 
-    private final StreamsMetricsImpl streamsMetrics = createMock(StreamsMetricsImpl.class);
-    private final Sensor expectedSensor = mock(Sensor.class);
+    private Sensor expectedSensor = Mockito.mock(Sensor.class);
     private final Map<String, String> tagMap = mkMap(mkEntry("key", "value"));
 
     @Test
     public void shouldGetHitRatioSensorWithBuiltInMetricsVersionCurrent() {
         final String hitRatio = "hit-ratio";
-        mockStatic(StreamsMetricsImpl.class);
-        expect(streamsMetrics.version()).andStubReturn(Version.LATEST);
-        expect(streamsMetrics.cacheLevelSensor(THREAD_ID, TASK_ID, STORE_NAME, hitRatio, RecordingLevel.DEBUG))
-            .andReturn(expectedSensor);
-        expect(streamsMetrics.cacheLevelTagMap(THREAD_ID, TASK_ID, STORE_NAME)).andReturn(tagMap);
+        final StreamsMetricsImpl streamsMetrics = Mockito.mock(StreamsMetricsImpl.class);
+        Mockito.when(streamsMetrics.cacheLevelSensor(THREAD_ID, TASK_ID, STORE_NAME, hitRatio, RecordingLevel.DEBUG)).thenReturn(expectedSensor);
+        Mockito.when(streamsMetrics.cacheLevelTagMap(THREAD_ID, TASK_ID, STORE_NAME)).thenReturn(tagMap);
         StreamsMetricsImpl.addAvgAndMinAndMaxToSensor(
-            expectedSensor,
-            StreamsMetricsImpl.CACHE_LEVEL_GROUP,
-            tagMap,
-            hitRatio,
-            HIT_RATIO_AVG_DESCRIPTION,
-            HIT_RATIO_MIN_DESCRIPTION,
-            HIT_RATIO_MAX_DESCRIPTION);
-        replay(streamsMetrics);
-        replay(StreamsMetricsImpl.class);
+                expectedSensor,
+                StreamsMetricsImpl.CACHE_LEVEL_GROUP,
+                tagMap,
+                hitRatio,
+                HIT_RATIO_AVG_DESCRIPTION,
+                HIT_RATIO_MIN_DESCRIPTION,
+                HIT_RATIO_MAX_DESCRIPTION);

Review comment:
       nit
   ```suggestion
               expectedSensor,
               StreamsMetricsImpl.CACHE_LEVEL_GROUP,
               tagMap,
               hitRatio,
               HIT_RATIO_AVG_DESCRIPTION,
               HIT_RATIO_MIN_DESCRIPTION,
               HIT_RATIO_MAX_DESCRIPTION
           );
   ```




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10835: KAFKA12905: Replace EasyMock and PowerMock with Mockito for NamedCacheMetricsTest

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10835:
URL: https://github.com/apache/kafka/pull/10835#discussion_r646771214



##########
File path: build.gradle
##########
@@ -1710,6 +1710,7 @@ project(':streams') {
     testImplementation libs.powermockEasymock

Review comment:
       Can we remove the powermock dependencies or are there other tests in Streams using 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org