cadonna commented on a change in pull request #10529:
URL: https://github.com/apache/kafka/pull/10529#discussion_r612266039



##########
File path: 
streams/src/test/java/org/apache/kafka/streams/processor/internals/InternalTopicManagerTest.java
##########
@@ -153,7 +156,8 @@ public void shouldNotCreateTopicsWithEmptyInput() throws 
Exception {
 
     @Test
     public void shouldOnlyRetryNotSuccessfulFuturesDuringSetup() {
-        final AdminClient admin = EasyMock.createNiceMock(AdminClient.class);
+        final AdminClient admin = EasyMock.createStrictMock(AdminClient.class);
+        config.put(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG, 10_000L);

Review comment:
       I think a default mock would be enough to get a better error message. 
Strict mocks also check the order of the calls which is not needed here. 
   
   I would also pass `new MockTime(1)` instead of increasing the timeout, 
because the test retrieves the time twice. `new MockTime(1)` would progress 
time 1 ms each time. Hence, we would be within the 100 ms of the global 
`max.poll.interval.ms` and since we control the time progression the test would 
be more robust than by increasing `max.poll.interval.ms` to a higher value. 
With `new MockTime(1)`, we also avoid running the test indefinitely in case of 
a mistake in the production code, since after 50 time retrievals, the test 
would run into a timeout and the admin client mock would throw the unexpected 
method call error. Probably, it would be even better to use something like `new 
MockTime((Integer) 
config.get(StreamsConfig.consumerPrefix(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG))
 / 3)`.




-- 
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:
us...@infra.apache.org


Reply via email to