You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/08/16 15:51:15 UTC
[GitHub] [kafka] mimaison commented on a diff in pull request #12473: KAFKA-13133: Replace EasyMock and PowerMock with Mockito for AbstractHerderTest
mimaison commented on code in PR #12473:
URL: https://github.com/apache/kafka/pull/12473#discussion_r946860309
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -141,89 +142,52 @@ public class AbstractHerderTest {
private final String connector = "connector";
private final ConnectorClientConfigOverridePolicy noneConnectorClientConfigOverridePolicy = new NoneConnectorClientConfigOverridePolicy();
- @MockStrict private Worker worker;
- @MockStrict private WorkerConfigTransformer transformer;
- @MockStrict private ConfigBackingStore configStore;
- @MockStrict private StatusBackingStore statusStore;
- @MockStrict private ClassLoader classLoader;
+ @Mock private Worker worker;
Review Comment:
I think we can also remove the `TASK_CONFIGS` constant above
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -556,28 +473,24 @@ public void testConfigValidationTransformsExtendResults() {
assertEquals("transforms.xformB.type", infos.get("transforms.xformB.type").configValue().name());
assertFalse(infos.get("transforms.xformB.type").configValue().errors().isEmpty());
- verifyAll();
+ verify(plugins, times(2)).transformations();
+ verify(plugins).newConnector(connectorClass.getName());
+ verify(plugins).compareAndSwapLoaders(insConnector);
}
- @Test()
+ @Test
public void testConfigValidationPredicatesExtendResults() {
- AbstractHerder herder = createConfigValidationHerder(SampleSourceConnector.class, noneConnectorClientConfigOverridePolicy);
+ final Class<? extends Connector> connectorClass = SampleSourceConnector.class;
+ AbstractHerder herder = createConfigValidationHerder(connectorClass, noneConnectorClientConfigOverridePolicy);
// 2 transform aliases defined -> 2 plugin lookups
- Set<PluginDesc<Transformation<?>>> transformations = new HashSet<>();
- transformations.add(transformationPluginDesc());
- EasyMock.expect(plugins.transformations()).andReturn(transformations).times(1);
-
- Set<PluginDesc<Predicate<?>>> predicates = new HashSet<>();
- predicates.add(predicatePluginDesc());
- EasyMock.expect(plugins.predicates()).andReturn(predicates).times(2);
-
- replayAll();
+ when(plugins.transformations()).thenReturn(Collections.singleton(transformationPluginDesc()));
+ when(plugins.predicates()).thenReturn(Collections.singleton(predicatePluginDesc()));
// Define 2 transformations. One has a class defined and so can get embedded configs, the other is missing
Review Comment:
I guess it should be `Define 2 predicates`?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -412,38 +334,32 @@ public void testBuildRestartPlanForNoRestart() {
assertFalse(restartPlan.shouldRestartConnector());
assertFalse(restartPlan.shouldRestartTasks());
assertTrue(restartPlan.taskIdsToRestart().isEmpty());
-
- PowerMock.verifyAll();
}
@Test
public void testConfigValidationEmptyConfig() {
AbstractHerder herder = createConfigValidationHerder(SampleSourceConnector.class, noneConnectorClientConfigOverridePolicy, 0);
- replayAll();
assertThrows(BadRequestException.class, () -> herder.validateConnectorConfig(Collections.emptyMap(), false));
+ verify(worker, times(2)).configTransformer();
Review Comment:
I wonder if it makes more sense to check for `verify(transformer).transform(Collections.emptyMap());` instead. The fact we call `configTransformer()` twice is kind of an implementation detail.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -236,31 +200,21 @@ public void connectorStatus() {
assertEquals(0, taskState.id());
assertEquals("UNASSIGNED", taskState.state());
assertEquals(workerId, taskState.workerId());
-
- PowerMock.verifyAll();
}
@Test
- public void taskStatus() {
+ public void testTaskStatus() {
ConnectorTaskId taskId = new ConnectorTaskId("connector", 0);
Review Comment:
We can use the `connector` variable instead of the literal here
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -141,89 +142,52 @@ public class AbstractHerderTest {
private final String connector = "connector";
private final ConnectorClientConfigOverridePolicy noneConnectorClientConfigOverridePolicy = new NoneConnectorClientConfigOverridePolicy();
- @MockStrict private Worker worker;
- @MockStrict private WorkerConfigTransformer transformer;
- @MockStrict private ConfigBackingStore configStore;
- @MockStrict private StatusBackingStore statusStore;
- @MockStrict private ClassLoader classLoader;
+ @Mock private Worker worker;
+ @Mock private WorkerConfigTransformer transformer;
+ @Mock private ConfigBackingStore configStore;
+ @Mock private StatusBackingStore statusStore;
+ @Mock private ClassLoader classLoader;
@Mock private Plugins plugins;
+ private ClassLoader loader;
+ private Connector insConnector;
Review Comment:
Is there a reason it's called `insConnector`? I wonder if renaming the other `connector` to something like `connName` would make more sense. WDYT?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -556,28 +473,24 @@ public void testConfigValidationTransformsExtendResults() {
assertEquals("transforms.xformB.type", infos.get("transforms.xformB.type").configValue().name());
assertFalse(infos.get("transforms.xformB.type").configValue().errors().isEmpty());
- verifyAll();
+ verify(plugins, times(2)).transformations();
+ verify(plugins).newConnector(connectorClass.getName());
+ verify(plugins).compareAndSwapLoaders(insConnector);
}
- @Test()
+ @Test
public void testConfigValidationPredicatesExtendResults() {
- AbstractHerder herder = createConfigValidationHerder(SampleSourceConnector.class, noneConnectorClientConfigOverridePolicy);
+ final Class<? extends Connector> connectorClass = SampleSourceConnector.class;
+ AbstractHerder herder = createConfigValidationHerder(connectorClass, noneConnectorClientConfigOverridePolicy);
// 2 transform aliases defined -> 2 plugin lookups
- Set<PluginDesc<Transformation<?>>> transformations = new HashSet<>();
- transformations.add(transformationPluginDesc());
- EasyMock.expect(plugins.transformations()).andReturn(transformations).times(1);
-
- Set<PluginDesc<Predicate<?>>> predicates = new HashSet<>();
- predicates.add(predicatePluginDesc());
- EasyMock.expect(plugins.predicates()).andReturn(predicates).times(2);
-
- replayAll();
+ when(plugins.transformations()).thenReturn(Collections.singleton(transformationPluginDesc()));
Review Comment:
Not sure if these comments still make sense
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org