mdedetrich commented on code in PR #12728:
URL: https://github.com/apache/kafka/pull/12728#discussion_r1005833555
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##########
@@ -123,91 +134,91 @@ private enum SourceSink {
private final ConnectorClientConfigOverridePolicy
noneConnectorClientConfigOverridePolicy = new
NoneConnectorClientConfigOverridePolicy();
+ private MockedStatic<Plugins> pluginsStatic;
+
+ private MockedStatic<WorkerConnector> workerConnectorStatic;
@Before
public void setup() {
- worker = PowerMock.createMock(Worker.class);
- String[] methodNames = new String[]{"connectorTypeForClass"/*,
"validateConnectorConfig"*/, "buildRestartPlan", "recordRestarting"};
- herder = PowerMock.createPartialMock(StandaloneHerder.class,
methodNames,
- worker, WORKER_ID, KAFKA_CLUSTER_ID, statusBackingStore, new
MemoryConfigBackingStore(transformer), noneConnectorClientConfigOverridePolicy);
+ worker = mock(Worker.class);
+ herder = mock(StandaloneHerder.class, withSettings()
+ .useConstructor(worker, WORKER_ID, KAFKA_CLUSTER_ID,
statusBackingStore, new MemoryConfigBackingStore(transformer),
noneConnectorClientConfigOverridePolicy)
+ .defaultAnswer(CALLS_REAL_METHODS));
createCallback = new FutureCallback<>();
- plugins = PowerMock.createMock(Plugins.class);
- pluginLoader = PowerMock.createMock(PluginClassLoader.class);
- delegatingLoader = PowerMock.createMock(DelegatingClassLoader.class);
- PowerMock.mockStatic(Plugins.class);
- PowerMock.mockStatic(WorkerConnector.class);
- Capture<Map<String, String>> configCapture = Capture.newInstance();
- EasyMock.expect(transformer.transform(eq(CONNECTOR_NAME),
EasyMock.capture(configCapture))).andAnswer(configCapture::getValue).anyTimes();
+ plugins = mock(Plugins.class);
+ pluginLoader = mock(PluginClassLoader.class);
+ delegatingLoader = mock(DelegatingClassLoader.class);
+ pluginsStatic = mockStatic(Plugins.class);
+ workerConnectorStatic = mockStatic(WorkerConnector.class);
+ final ArgumentCaptor<Map<String, String>> configCapture =
ArgumentCaptor.forClass(Map.class);
+ when(transformer.transform(eq(CONNECTOR_NAME),
configCapture.capture())).thenAnswer(invocation -> configCapture.getValue());
+ }
+
+ @After
+ public void tearDown() {
+ pluginsStatic.close();
+ workerConnectorStatic.close();
}
@Test
public void testCreateSourceConnector() throws Exception {
- connector = PowerMock.createMock(BogusSourceConnector.class);
+ connector = mock(BogusSourceConnector.class);
expectAdd(SourceSink.SOURCE);
Map<String, String> config = connectorConfig(SourceSink.SOURCE);
- Connector connectorMock = PowerMock.createMock(SourceConnector.class);
+ Connector connectorMock = mock(SourceConnector.class);
expectConfigValidation(connectorMock, true, config);
- PowerMock.replayAll();
-
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
createCallback);
Herder.Created<ConnectorInfo> connectorInfo =
createCallback.get(1000L, TimeUnit.SECONDS);
assertEquals(createdInfo(SourceSink.SOURCE), connectorInfo.result());
-
- PowerMock.verifyAll();
}
@Test
public void testCreateConnectorFailedValidation() {
// Basic validation should be performed and return an error, but
should still evaluate the connector's config
- connector = PowerMock.createMock(BogusSourceConnector.class);
+ connector = mock(BogusSourceConnector.class);
Map<String, String> config = connectorConfig(SourceSink.SOURCE);
config.remove(ConnectorConfig.NAME_CONFIG);
- Connector connectorMock = PowerMock.createMock(SourceConnector.class);
-
EasyMock.expect(worker.configTransformer()).andReturn(transformer).times(2);
- final Capture<Map<String, String>> configCapture =
EasyMock.newCapture();
-
EasyMock.expect(transformer.transform(EasyMock.capture(configCapture))).andAnswer(configCapture::getValue);
- EasyMock.expect(worker.getPlugins()).andReturn(plugins).times(3);
-
EasyMock.expect(plugins.compareAndSwapLoaders(connectorMock)).andReturn(delegatingLoader);
-
EasyMock.expect(plugins.newConnector(EasyMock.anyString())).andReturn(connectorMock);
+ Connector connectorMock = mock(SourceConnector.class);
+ when(worker.configTransformer()).thenReturn(transformer);
+ final ArgumentCaptor<Map<String, String>> configCapture =
ArgumentCaptor.forClass(Map.class);
+
when(transformer.transform(configCapture.capture())).thenAnswer(invocation ->
configCapture.getValue());
+ when(worker.getPlugins()).thenReturn(plugins);
+
when(plugins.compareAndSwapLoaders(connectorMock)).thenReturn(delegatingLoader);
+ when(plugins.newConnector(anyString())).thenReturn(connectorMock);
- EasyMock.expect(connectorMock.config()).andStubReturn(new ConfigDef());
+ when(connectorMock.config()).thenReturn(new ConfigDef());
ConfigValue validatedValue = new ConfigValue("foo.bar");
- EasyMock.expect(connectorMock.validate(config)).andReturn(new
Config(singletonList(validatedValue)));
-
EasyMock.expect(Plugins.compareAndSwapLoaders(delegatingLoader)).andReturn(pluginLoader);
-
- PowerMock.replayAll();
+ when(connectorMock.validate(config)).thenReturn(new
Config(singletonList(validatedValue)));
+
when(Plugins.compareAndSwapLoaders(delegatingLoader)).thenReturn(pluginLoader);
Review Comment:
So in the commit `Create static mock on Herder's connectorExecutor and
remove other unneeded mocks` I have pushed a solution to this problem from a
couple of ideas I had. Basically the idea is that we use the `AbstractHeaders`
`connectorExecutor` to create a `mockStatic` with the plugins class, if you put
a breakpoint on `Thread.currentThread()::setContextClassLoader` you can verify
that during the it never gets executed which means that
`Plugins::compareAndSwapLoaders` ends up having no effect (so its not
interacting with the mocks in any meaningful way).
Interestingly (and likely due to Mockito's `mockStatic` implementation), the
`pluginsStatic.when(() ->
Plugins.compareAndSwapLoaders(delegatingLoader)).thenReturn(pluginLoader);` and
`pluginsStatic.when(() ->
Plugins.compareAndSwapLoaders(delegatingLoader)).thenReturn(pluginLoader);` is
no longer needed (in fact the `pluginLoader` mock in general is not even
needed). I am still not entirely 100% sure about this because the debugger is
still claiming that the original `Plugins::compareAndSwapLoaders` was being
called even using `pluginsStatic.when(() ->
Plugins.compareAndSwapLoaders(delegatingLoader)).thenReturn(pluginLoader);` on
the same thread (and I tried different variations and I never managed to get
`pluginsStatic.when(() ->
Plugins.compareAndSwapLoaders(delegatingLoader)).thenReturn(pluginLoader);` to
work the same way EasyMock did in this regard) so it can easily be that the
core reason why this is working is that
```java
((AbstractHerder) herder).connectorExecutor.submit(() ->
pluginsStatic = mockStatic(Plugins.class)
).get()
```
is causing the base Mock to be on the same thread which causes the condition
`if (!current.equals(loader))` in `Plugins::compareAndSwapLoaders` to always be
`false` ultimately causing the `public static ClassLoader
compareAndSwapLoaders(ClassLoader loader)` method to not have any effect. I
don't know for certain if this is the case since we are dealing with black
magic mocking reflection shenanigans but what I do know is that at least we
aren't replacing the classloader with a mock which was your primary concern.
--
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
