KAFKA-10789: Streamlining Tests in ChangeLoggingKeyValueBytesStoreTest #18816
Conversation
JUnit5 test classes and methods should have default package visibility
Local variables should not shadow class fields
All unittest passed
github-actions
bot
added
triage
leaf-soba
changed the title
|
Hi @ijuma, @chia7712, @gongxuanzhang, could you please take a look? |
|
Pr needs a tag about Ci, I can't do it. sorry |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
fix import static org.mockito.ArgumentMatchers.anyString;
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
|
Hi @chia7712, in the CI process, there are three flaky tests that are unrelated to this PR. Could you let me know how to re-run the CI process to get them to pass? |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| @ExtendWith(MockitoExtension.class) | ||
| @MockitoSettings(strictness = Strictness.STRICT_STUBS) | ||
| public class ChangeLoggingKeyValueBytesStoreTest { | ||
| class ChangeLoggingKeyValueBytesStoreTest { |
There was a problem hiding this comment.
Can a test class be package private?
There was a problem hiding this comment.
In JUnit 5, test classes and methods can have default (package-private) visibility—they don’t need to be public to be recognized and executed. However, I noticed that most of the existing Kafka unit tests still use public, so I’ll follow the current convention and revert the change for consistency.
There was a problem hiding this comment.
Thanks to the info. Interesting. Did not know that. -- Guess we use public historically, and I also don't see much value to change it to package private -- sounds like a not of unnecessary noise on PRs, w/o any clear benefit.
.../test/java/org/apache/kafka/streams/state/internals/ChangeLoggingKeyValueBytesStoreTest.java
Show resolved
Hide resolved
| mockMap.put(invocation.getArgument(0), invocation.getArgument(1)); | ||
| StoreQueryUtils.updatePosition(innerMock.getPosition(), context); | ||
| return null; | ||
| }).when(innerMock).put(any(Bytes.class), any(byte[].class)); |
There was a problem hiding this comment.
Why do we use doAnswer(...).when(...) here?
In mockGet we use when(...).thanAnswer(...) what I find much easier to read.
There was a problem hiding this comment.
Thanks for the question!
We use doAnswer(...).when(...) here because put is a void method. Mockito requires this syntax for mocking void methods — when(...).thenAnswer(...) only works for methods that return a value.
In contrast, get returns a value, so we can use when(...).thenAnswer(...) there, which I agree is a bit more readable.
@Override
public synchronized void put(final Bytes key, final byte[] value) {
putInternal(key, value);
}
| private void mockPosition() { | ||
| when(innerMock.getPosition()).thenReturn(Position.emptyPosition()); | ||
| } | ||
| private void mockGet(final Map<Bytes, byte[]> mockMap) { |
There was a problem hiding this comment.
Why are we using mockMap? It seems unnecessarily complex? -- It seems much more straightforward to just "expect" calls into the innerMock store?
There was a problem hiding this comment.
Thanks for pointing that out!
We're using mockMap here to simulate the internal state of the underlying store, since the tests in CachingInMemoryKeyValueStoreTest are being streamlined to rely on a mocked underlyingStore as per the KIP-614 review discussion.
Because innerMock is a mock and doesn’t retain state on its own, we use mockMap to mimic how the real store would behave across multiple interactions — especially for verifying behavior like reads after writes. This lets us preserve the semantics of the store while still keeping the actual store mocked, as requested.
That said, I'm open to simplifying it further if there's a cleaner way to preserve the same test coverage and behavior expectations.
| public void shouldDelegateInit() { | ||
| final InternalMockProcessorContext context = mockContext(); | ||
| final KeyValueStore<Bytes, byte[]> innerMock = mock(InMemoryKeyValueStore.class); | ||
| void shouldDelegateInit() { |
There was a problem hiding this comment.
Same question as above for the class: I thought test methods must be public?
There was a problem hiding this comment.
In JUnit 5, test classes and methods can have default (package-private) visibility—they don’t need to be public to be recognized and executed. However, I noticed that most of the existing Kafka unit tests still use public, so I’ll follow the current convention and revert the change for consistency.
revert remove unnecessary public
insert empty lines between methods
InMemoryKeyValueStoreto streamlined the unit testpublicand rename the variable inside the method with a different name with outside of the method.Committer Checklist (excluded from commit message)