vvcephei commented on a change in pull request #8896:
URL: https://github.com/apache/kafka/pull/8896#discussion_r443005504
##########
File path:
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java
##########
@@ -223,6 +227,7 @@ public void
shouldInitializeChangelogAndCheckForCompletion() {
@Test
public void shouldPollWithRightTimeout() {
EasyMock.expect(storeMetadata.offset()).andReturn(null).andReturn(9L).anyTimes();
+
EasyMock.expect(stateManager.changelogOffsets()).andReturn(singletonMap(tp,
5L));
Review comment:
I didn't think to do this... This might be equivocation, but it seems
like if I wrote that in a code comment, it may or may not be true in the
future. Looking at the tests, there are already like a dozen cryptic, redundant
mocks, so I'm not sure justifying this one really makes a material impact on
this test's readability, which is already approaching zero.
Adding a comment like "this is just to prevent the logger from throwing an
NPE" carries the risk that it can quickly become untrue in two ways:
1. Maybe we remove or change the log so that it wouldn't need this mock;
since it's a "nice" mock, we'll never know. In fact, I can't verify this call
because the way the logger is configured only to print every ten seconds makes
the NPE nondeterministic. Plus, it's not great to verify stuff that is beside
the point of the test.
2. Maybe we change the implementation so that it actually does exercise this
mocked behavior, then the comment will become untrue, but we may not even
notice.
Typically, having this many specific and complex mocks in a test indicates
that we shouldn't be using easymock, but instead configure the component with
"dummy" state manager, etc. If we re-wrote this test to use that strategy, then
we wouldn't need to make explicit expectations like this.
Anyway, that's why I'm sort of inclined on just declaring bankruptcy on the
comprehensibility of this test.
----------------------------------------------------------------
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:
[email protected]
