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
