[GitHub] [kafka] dplavcic commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests

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

dplavcic commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r944781108


##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
             Serdes.String(), new MockTime()));
     }
 
-    @Test
-    public void shouldThrowNullPointerIfKeySerdeIsNull() {
-        assertThrows(NullPointerException.class, () -> new KeyValueStoreBuilder<>(supplier, null, Serdes.String(), new MockTime()));
-    }
-
-    @Test
-    public void shouldThrowNullPointerIfValueSerdeIsNull() {
-        assertThrows(NullPointerException.class, () -> new KeyValueStoreBuilder<>(supplier, Serdes.String(), null, new MockTime()));
-    }

Review Comment:
   Please check previous comment for explanation.
   
   ```text
   Expected: "value serde cannot be null"
        but: was "name cannot be null"
   ```



##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
             Serdes.String(), new MockTime()));
     }
 
-    @Test

Review Comment:
   Nice catch!
   
   The thing is that `supplier.name()` returns `null` and there is an explicit null check on the `name` value in the `AbstractStoreBuilder` class.
   
   
   If we update this test and assert/verify error message:
   ```java
       @Test
       public void shouldThrowNullPointerIfKeySerdeIsNull() {
           final Exception e = assertThrows(NullPointerException.class, () -> new KeyValueStoreBuilder<>(supplier, null, Serdes.String(), new MockTime()));
           assertThat(e.getMessage(), equalTo("serde cannot be null"));
       }
   ```
   
   We will see that it fails with following message:
   ```text
   Expected: "serde cannot be null"
        but: was "name cannot be null"
   ```
   
   The question to someone more familiar with the project is:
   - should we add `null` check in the `KeyValueStoreBuilder` constructor?



##########
streams/src/test/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProviderTest.java:
##########
@@ -449,22 +454,18 @@ private StreamTask createStreamsTask(final StreamsConfig streamsConfig,
             new TopologyConfig(null, streamsConfig, new Properties()).getTaskConfig(),
             streamsMetrics,
             stateDirectory,
-            EasyMock.createNiceMock(ThreadCache.class),
+            mock(ThreadCache.class),
             new MockTime(),
             stateManager,
             recordCollector,
             context, logContext);
     }
 
     private void mockThread(final boolean initialized) {
-        EasyMock.expect(threadMock.isRunning()).andReturn(initialized);
-        EasyMock.expect(threadMock.allTasks()).andStubReturn(tasks);
-        EasyMock.expect(threadMock.activeTaskMap()).andStubReturn(tasks);

Review Comment:
   I guess first three stubs are no longer needed or?



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