mdedetrich commented on code in PR #12781:
URL: https://github.com/apache/kafka/pull/12781#discussion_r1006543034
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java:
##########
@@ -547,32 +538,18 @@ public void
testReadEndOffsetsUsingAdminThatFailsWithRetriable() throws Exceptio
private void setupWithAdmin() {
Supplier<TopicAdmin> adminSupplier = () -> admin;
java.util.function.Consumer<TopicAdmin> initializer = admin -> { };
- store = PowerMock.createPartialMock(KafkaBasedLog.class, new
String[]{"createConsumer", "createProducer"},
- TOPIC, PRODUCER_PROPS, CONSUMER_PROPS, adminSupplier,
consumedCallback, time, initializer);
+ store = new MockedKafkaBasedLog(TOPIC, PRODUCER_PROPS, CONSUMER_PROPS,
adminSupplier, consumedCallback, time, initializer);
}
- private void expectProducerAndConsumerCreate() throws Exception {
- PowerMock.expectPrivate(store, "createProducer")
- .andReturn(producer);
- PowerMock.expectPrivate(store, "createConsumer")
- .andReturn(consumer);
- }
-
- private void expectStart() throws Exception {
+ private void expectStart() {
initializer.run();
- EasyMock.expectLastCall().times(1);
-
- expectProducerAndConsumerCreate();
+ verify(initializer, times(1)).run();
}
private void expectStop() {
producer.close();
- PowerMock.expectLastCall();
+ verify(producer, times(1)).close();
Review Comment:
> Does the producer.close() call trivially satisfy the following verify()
call?
So to be honest I don't know whats going on here. The literal translation of
```java
producer.close();
PowerMock.expectLastCall();
```
to Mockito is
```java
producer.close();
verify(producer, times(1)).close();
```
So to answer your question this does seem fishy because of course we call
`producer.close()` we expect it to be called unless some exception is thrown
(which would fail the test otherwise).
> Also it looks like this method is not really translated from easymock yet,
as it is separate from the store.stop() calls throughout the test. I think that
instead of calling store.stop(), the test should call expectStop, and then the
expectStop calls store.stop and asserts that the shutdown procedure happens.
Are you alluding to the fact that the equivalent `PowerMock.verifyAll();` is
missing here? If thats so then yes this is indeed the case and this is one of
the problems with PowerMock/EasyMock is that its not that explicit, i.e.
`PowerMock.verifyAll()` will verify methods that are directly called (which
almost all of the time is pointless) where as ideally you are meant to use
`verify` on methods that are called indirectly called by tests to ensure that
they are being executed.
So I guess to answer your test more directly, I can see what you are saying
but I would like to know what exactly a "shutdown procedure" actually means in
terms of mocking, i.e. what methods (aside from `store.stop()` which we
directly call) should we be verifying?
--
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
