C0urante commented on code in PR #12728:
URL: https://github.com/apache/kafka/pull/12728#discussion_r1003623254
##########
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:
I did some research and it turns out that Mockito's static mocking API only
supports setting expectations on the same thread that the `MockedStatic` was
created on (see https://github.com/mockito/mockito/issues/2142).
I'm afraid to install a mocked classloader as the context classloader, even
in tests, which is what's happening right now (you can verify this by adding a
breakpoint in `Plugins::compareAndSwapLoaders` at the line that calls
`Thread.currentThread()::setContextClassLoader` and running the tests through a
debugger).
Can you see if there's a way to avoid installing a mocked classloader as the
context classloader in these tests? Feel free to make small tweaks to
non-testing code if necessary.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##########
@@ -319,35 +318,28 @@ public void testRestartConnectorNewTaskConfigs() throws
Exception {
Map<String, String> config = connectorConfig(SourceSink.SOURCE);
ConnectorTaskId taskId = new ConnectorTaskId(CONNECTOR_NAME, 0);
- Connector connectorMock = PowerMock.createMock(SourceConnector.class);
+ Connector connectorMock = mock(SourceConnector.class);
expectConfigValidation(connectorMock, true, config);
worker.stopAndAwaitConnector(CONNECTOR_NAME);
- EasyMock.expectLastCall();
+ doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
Review Comment:
I tried to highlight lines 314 and 315; GitHub apparently doesn't support
that. Both of those lines should be removed and replaced with the `verify`
invocation, not just line 315.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##########
@@ -319,35 +318,28 @@ public void testRestartConnectorNewTaskConfigs() throws
Exception {
Map<String, String> config = connectorConfig(SourceSink.SOURCE);
ConnectorTaskId taskId = new ConnectorTaskId(CONNECTOR_NAME, 0);
- Connector connectorMock = PowerMock.createMock(SourceConnector.class);
+ Connector connectorMock = mock(SourceConnector.class);
expectConfigValidation(connectorMock, true, config);
worker.stopAndAwaitConnector(CONNECTOR_NAME);
- EasyMock.expectLastCall();
+ doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
- Capture<Callback<TargetState>> onStart = EasyMock.newCapture();
- worker.startConnector(eq(CONNECTOR_NAME), eq(config),
EasyMock.anyObject(HerderConnectorContext.class),
- eq(herder), eq(TargetState.STARTED),
EasyMock.capture(onStart));
- EasyMock.expectLastCall().andAnswer(() -> {
+ final ArgumentCaptor<Callback<TargetState>> onStart =
ArgumentCaptor.forClass(Callback.class);
+ doAnswer(invocation -> {
onStart.getValue().onCompletion(null, TargetState.STARTED);
return true;
- });
+ }).when(worker).startConnector(eq(CONNECTOR_NAME), eq(config),
any(HerderConnectorContext.class),
+ eq(herder), eq(TargetState.STARTED), onStart.capture());
-
EasyMock.expect(worker.connectorNames()).andReturn(Collections.singleton(CONNECTOR_NAME));
- EasyMock.expect(worker.getPlugins()).andReturn(plugins);
+
when(worker.connectorNames()).thenReturn(Collections.singleton(CONNECTOR_NAME));
+ when(worker.getPlugins()).thenReturn(plugins);
// changed task configs, expect a new set of tasks to be brought up
(and the old ones to be stopped)
Map<String, String> taskConfigs = taskConfig(SourceSink.SOURCE);
taskConfigs.put("k", "v");
- EasyMock.expect(worker.connectorTaskConfigs(CONNECTOR_NAME, new
SourceConnectorConfig(plugins, config,
true))).andReturn(Collections.singletonList(taskConfigs));
+ when(worker.connectorTaskConfigs(CONNECTOR_NAME, new
SourceConnectorConfig(plugins, config, true)))
+ .thenReturn(Collections.singletonList(taskConfigs));
- worker.stopAndAwaitTasks(Collections.singletonList(taskId));
Review Comment:
RE `taskId`: I mentioned removing that field because it wasn't used in the
current commit; if it becomes necessary (or at least, useful) because of a
different change, feel free to add it back.
RE verifying method invocation: yes, this is still necessary. A major part
of this test is verifying that we interact with the `Worker` instance correctly
when a request to restart a connector is picked up and then the restarted
connector generates new task configs. This includes stopping it and its tasks,
and then starting them again. Right now, this PR doesn't preserve all of the
guarantees about those interactions that we got with the EasyMock/PowerMock
tests; we should do our best not to drop any of those guarantees during the
Migration.
RE reordering: this test case performs two invocations on the herder, to
create and then restart a connector. The first invocation is supposed to
generate one set of task configs, and the second is supposed to generate a
different set. The call to `expectAdd` at line 308 sets up mocking behavior for
the `Worker` instance's `startConnector`, `isRunning`, `connectorTaskConfigs`,
and other methods, which are meant to be used for the first herder invocation
at line 334 where we call `putConnectorConfig`. Many (or even possibly all) of
the other expectations set up in this test case are meant to be used for the
second herder invocation at line 339 where we call `restartConnector`. I
believe the second set of expectations is actually overwriting the first set,
and the reason that that's not causing the test to fail is that we aren't
verifying the correct interactions with the `Worker` instance.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##########
@@ -218,43 +229,37 @@ public void testCreateConnectorAlreadyExists() throws
Throwable {
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
failedCreateCallback);
ExecutionException exception = assertThrows(ExecutionException.class,
() -> failedCreateCallback.get(1000L, TimeUnit.SECONDS));
assertEquals(AlreadyExistsException.class,
exception.getCause().getClass());
- PowerMock.verifyAll();
}
@Test
public void testCreateSinkConnector() throws Exception {
- connector = PowerMock.createMock(BogusSinkConnector.class);
+ connector = mock(BogusSinkConnector.class);
expectAdd(SourceSink.SINK);
Map<String, String> config = connectorConfig(SourceSink.SINK);
- Connector connectorMock = PowerMock.createMock(SinkConnector.class);
+ Connector connectorMock = mock(SinkConnector.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.SINK), connectorInfo.result());
-
- PowerMock.verifyAll();
}
@Test
public void testDestroyConnector() 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);
-
EasyMock.expect(statusBackingStore.getAll(CONNECTOR_NAME)).andReturn(Collections.emptyList());
+
when(statusBackingStore.getAll(CONNECTOR_NAME)).thenReturn(Collections.emptyList());
statusBackingStore.put(new ConnectorStatus(CONNECTOR_NAME,
AbstractStatus.State.DESTROYED, WORKER_ID, 0));
statusBackingStore.put(new TaskStatus(new
ConnectorTaskId(CONNECTOR_NAME, 0), TaskStatus.State.DESTROYED, WORKER_ID, 0));
Review Comment:
Yes, and please call out, tweak, and/or remove anything else like this that
seems unusual.
--
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
