-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-16652: add unit test for ClusterTemplate offering zero ClusterConfig #15862
Conversation
core/src/test/java/kafka/test/junit/ClusterTestExtensionsUnitTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TaiJuWu thanks for updated PR.
core/src/test/java/kafka/test/junit/ClusterTestExtensionsUnitTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/kafka/test/junit/ClusterTestExtensionsUnitTest.java
Outdated
Show resolved
Hide resolved
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
public class ClusterTestExtensionsUnitTest { | ||
|
||
static class StubTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this nested class is unnecessary. For example:
static List<ClusterConfig> cfgEmpty() {
return new ArrayList<ClusterConfig>();
}
@SuppressWarnings({"unchecked", "rawtypes"})
private ExtensionContext buildExtensionContext(String methodName) throws Exception {
ExtensionContext extensionContext = mock(ExtensionContext.class);
Class clazz = ClusterTestExtensionsUnitTest.class;
Method method = clazz.getDeclaredMethod(methodName);
when(extensionContext.getRequiredTestClass()).thenReturn(clazz);
when(extensionContext.getRequiredTestMethod()).thenReturn(method);
return extensionContext;
}
BTW, methodName
is always "cfgEmpty", so maybe we can remove it from input argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, keeping the argument will make this function reusable if needed.
@TaiJuWu Could you please rebase code? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TaiJuWu nice patch! please take a look at one last comment :)
core/src/test/java/kafka/test/junit/ClusterTestExtensionsUnitTest.java
Outdated
Show resolved
Hide resolved
when(annot.value()).thenReturn("").thenReturn(" "); | ||
when(annot.value()).thenReturn("").thenReturn(" ").thenReturn("cfgEmpty"); | ||
|
||
Assertions.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is equal to next one, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the first one is for @clusterTemplate("")
, the other is @ClusterTemplate(" ")
.
…nfig (apache#15862) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
*More detailed description of your change,
refactor processClusterTemplate and add unit test for empty config
*Summary of testing strategy (including rationale)
Committer Checklist (excluded from commit message)