KAFKA-13133 Replace EasyMock and PowerMock with Mockito for AbstractHerderTest #11137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmccabe
reviewed
Aug 4, 2021
| EasyMock.expect(herder.rawConfig(connector)).andReturn(null); | ||
| EasyMock.expect(configStore.snapshot()).andReturn(SNAPSHOT); | ||
| replayAll(); | ||
| // AbstractHerder herder = partialMockBuilder(AbstractHerder.class) |
There was a problem hiding this comment.
it seems like you intended to remove this?
There was a problem hiding this comment.
Yes, I have removd.thx remind.
remove redundant code
|
@kkonstantine @rhauch It would be great to have a review from one of you two so that we can get this merged. |
ijuma
reviewed
Sep 6, 2021
| final private Plugins plugins = mock(Plugins.class); | ||
| final private ClassLoader classLoader = mock(ClassLoader.class); | ||
| final private ConfigBackingStore configStore = mock(ConfigBackingStore.class); | ||
| final private StatusBackingStore statusStore = mock(StatusBackingStore.class); |
There was a problem hiding this comment.
Nit: we usually write private final.
There was a problem hiding this comment.
Thanks for reminding,it has been modified and test also remove in build.gradle.
|
We should also remove this test from https://github.com/apache/kafka/blob/trunk/build.gradle#L381 before we merge this PR. |
…stractHerderTest
…kafka into testAbstractHerderTest
mimaison
reviewed
Oct 29, 2021
| new TaskStatus(taskId, AbstractStatus.State.UNASSIGNED, workerId, generation))); | ||
|
|
||
| replayAll(); | ||
| ConnectorStateInfo csi = herder.connectorStatus(connector); |
There was a problem hiding this comment.
Can we assert anything on csi? Otherwise, this test does not assert anything.
Do we actually need this test? It seems the test below is basically the same but with asserts?
| .defaultAnswer(CALLS_REAL_METHODS)); | ||
|
|
||
| when(herder.generation()).thenReturn(generation); | ||
| when(herder.rawConfig(connector)).thenReturn(null); |
There was a problem hiding this comment.
Do we actually have to mock generation() and rawConfig() for this test? Looking at connector(), it looks like it only relies on the snapshot.
There was a problem hiding this comment.
You are right @mimaison here and in your other comments about extra stubbing. When we run the test with @RunWith(MockitoJUnitRunner.class) which implicitly verifies that each mock is being invoked, the test fails.
Unnecessary stubbings detected in test class: AbstractHerderTest
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
1. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.createConfigValidationHerder(AbstractHerderTest.java:880)
2. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testConnectors(AbstractHerderTest.java:165)
3. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testConnectors(AbstractHerderTest.java:166)
4. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForUnknownConnector(AbstractHerderTest.java:255)
5. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testConnectorStatus(AbstractHerderTest.java:179)
6. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.connectorStatus(AbstractHerderTest.java:198)
7. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.connectorStatus(AbstractHerderTest.java:207)
8. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForConnectorAndTasks(AbstractHerderTest.java:278)
9. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForConnectorAndTasks(AbstractHerderTest.java:286)
10. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForNoRestart(AbstractHerderTest.java:314)
11. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForNoRestart(AbstractHerderTest.java:322)
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.
org.mockito.exceptions.misusing.UnnecessaryStubbingException:
Unnecessary stubbings detected in test class: AbstractHerderTest
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
1. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.createConfigValidationHerder(AbstractHerderTest.java:880)
2. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testConnectors(AbstractHerderTest.java:165)
3. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testConnectors(AbstractHerderTest.java:166)
4. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForUnknownConnector(AbstractHerderTest.java:255)
5. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testConnectorStatus(AbstractHerderTest.java:179)
6. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.connectorStatus(AbstractHerderTest.java:198)
7. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.connectorStatus(AbstractHerderTest.java:207)
8. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForConnectorAndTasks(AbstractHerderTest.java:278)
9. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForConnectorAndTasks(AbstractHerderTest.java:286)
10. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForNoRestart(AbstractHerderTest.java:314)
11. -> at org.apache.kafka.connect.runtime.AbstractHerderTest.testBuildRestartPlanForNoRestart(AbstractHerderTest.java:322)
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.
at app//org.mockito.internal.runners.StrictRunner.run(StrictRunner.java:53)
at app//org.mockito.junit.MockitoJUnitRunner.run(MockitoJUnitRunner.java:163)
at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
at java.base@11.0.14/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base@11.0.14/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base@11.0.14/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base@11.0.14/java.lang.reflect.Method.invoke(Method.java:566)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
at org.gradle.api.internal.tasks.testing.worker.TestWorker$2.run(TestWorker.java:176)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
I am submitting a new PR containing commits from this PR. I will ensure that your comments are addressed there.
| .useConstructor(worker, workerId, kafkaClusterId, statusStore, configStore, noneConnectorClientConfigOverridePolicy) | ||
| .defaultAnswer(CALLS_REAL_METHODS)); | ||
|
|
||
| when(herder.generation()).thenReturn(generation); |
There was a problem hiding this comment.
I don't think we need to mock generation() in this test.
|
|
||
| EasyMock.expect(herder.generation()).andStubReturn(generation); | ||
| EasyMock.expect(herder.rawConfig(connector)).andReturn(null); | ||
| when(herder.generation()).thenReturn(generation); |
There was a problem hiding this comment.
Same, I don't think it's needed in this test. I'm not going to comment on the other tests but let's remove the mocks we don't need.
| EasyMock.expect(worker.getPlugins()).andStubReturn(plugins); | ||
| when(statusStore.getAll(connector)) | ||
| .thenReturn(taskStatuses); | ||
| when(worker.getPlugins()).thenReturn(plugins); |
There was a problem hiding this comment.
I don't think this mock is needed. Same below.
|
|
||
| verifyAll(); | ||
| } | ||
|
|
||
| @Test() |
There was a problem hiding this comment.
We can drop the parenthesis here. Same below
| EasyMock.expect(plugins.transformations()).andReturn(transformations).times(2); | ||
|
|
||
| replayAll(); | ||
| when(plugins.transformations()).thenReturn(transformations); |
There was a problem hiding this comment.
What about inlining transformations and having something like:
when(plugins.transformations()).thenReturn(Collections.singleton(transformationPluginDesc()));
| public void testConfigValidationTransformsExtendResults() throws Throwable { | ||
| AbstractHerder herder = createConfigValidationHerder(TestSourceConnector.class, noneConnectorClientConfigOverridePolicy); | ||
| public void testConfigValidationTransformsExtendResults() { | ||
| final Class<? extends Connector> connectorClass = TestSourceConnector.class; |
There was a problem hiding this comment.
We have this variable in many tests, should instead have a field in this class?
|
|
||
| // 2 transform aliases defined -> 2 plugin lookups | ||
| Set<PluginDesc<Transformation<?>>> transformations = new HashSet<>(); | ||
| transformations.add(transformationPluginDesc()); | ||
| EasyMock.expect(plugins.transformations()).andReturn(transformations).times(1); | ||
| when(plugins.transformations()).thenReturn(transformations); |
There was a problem hiding this comment.
Same as mentioned above, because these Set instances only have 1 element, we can maybe inline these variables. WDYT?
|
@wycccccc are you planning to address the comments? |
|
Closing in favor of #12473. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Development of EasyMock and PowerMock has stagnated while Mockito continues to be actively developed. With the new Java cadence, it's a problem to depend on libraries that do bytecode generation and are not actively maintained. In addition, Mockito is also easier to use.KAFKA-7438
Committer Checklist (excluded from commit message)