adixitconfluent commented on code in PR #23468:
URL: https://github.com/apache/kafka/pull/23468#discussion_r4033422664
##########
core/src/test/java/kafka/server/share/DelayedShareFetchTest.java:
##########
@@ -3336,6 +3336,15 @@ private static BiConsumer<SharePartitionKey, Throwable>
mockExceptionHandler() {
return mock(BiConsumer.class);
}
+ private static SharePartition mockSharePartition() {
+ SharePartition sharePartition = Mockito.mock(SharePartition.class);
+
when(sharePartition.nextFetchOffsetIfAcquirable()).thenAnswer(invocation ->
+ sharePartition.canAcquireRecords()
+ ? OptionalLong.of(sharePartition.nextFetchOffset())
+ : OptionalLong.empty());
+ return sharePartition;
+ }
+
Review Comment:
The mock delegates `nextFetchOffsetIfAcquirable()` to `canAcquireRecords()`
+ `nextFetchOffset()`. But the real implementation does neither of these — it
calls `nextFetchOffsetUnderLock()` and checks fields directly. This means:
1. Tests continue to set up when(sp.canAcquireRecords()).thenReturn(true)
and when(sp.nextFetchOffset()).thenReturn(10L) as separate stubs, which works
for the mock but doesn't exercise the combined code path.
2. If a bug is introduced in the real nextFetchOffsetIfAcquirable() that
differs from canAcquireRecords() behavior, these mock-based tests won't catch
it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
