yashmayya commented on code in PR #14152: URL: https://github.com/apache/kafka/pull/14152#discussion_r1284245654
########## streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreBuilderTest.java: ########## @@ -164,17 +160,6 @@ public void shouldThrowNullPointerIfInnerIsNull() { assertThrows(NullPointerException.class, () -> new WindowStoreBuilder<>(null, Serdes.String(), Serdes.String(), new MockTime())); } - @Test - public void shouldThrowNullPointerIfKeySerdeIsNull() { - assertThrows(NullPointerException.class, () -> new WindowStoreBuilder<>(supplier, null, Serdes.String(), new MockTime())); - } - - @Test - public void shouldThrowNullPointerIfValueSerdeIsNull() { - assertThrows(NullPointerException.class, () -> new WindowStoreBuilder<>(supplier, Serdes.String(), - null, new MockTime())); - } Review Comment: These tests were buggy - there aren't currently any `null` checks in place for key / value serdes. The only reason they were passing earlier is due to the use of `EasyMock` with ["nice" mocks](https://easymock.org/user-guide.html#mocking-nice). In the `setUp` method, the expectation for the `WindowBytesStoreSupplier::name` method is setup - https://github.com/apache/kafka/blob/7782741262c08e5735f7c8e09727ec37cb5f7f02/streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreBuilderTest.java#L61 By default, this method stub covers only the first invocation of the method and subsequent invocations will result in a `null` value being returned (because the mock object is "nice"). The first invocation occurs in the `setUp` method itself when the builder is instantiated - https://github.com/apache/kafka/blob/7782741262c08e5735f7c8e09727ec37cb5f7f02/streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreBuilderTest.java#L65-L70 https://github.com/apache/kafka/blob/7782741262c08e5735f7c8e09727ec37cb5f7f02/streams/src/main/java/org/apache/kafka/streams/state/internals/WindowStoreBuilder.java#L39 The `null` check for name here - https://github.com/apache/kafka/blob/7782741262c08e5735f7c8e09727ec37cb5f7f02/streams/src/main/java/org/apache/kafka/streams/state/internals/AbstractStoreBuilder.java#L41 is what resulted in the `NullPointerException` being thrown in the `shouldThrowNullPointerIfKeySerdeIsNull` and `shouldThrowNullPointerIfValueSerdeIsNull` tests. This test bug was discovered because method stubbings in `Mockito` are applied to any number of invocations of the method by default. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org