soarez commented on code in PR #15862:
URL: https://github.com/apache/kafka/pull/15862#discussion_r1590420247
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -119,23 +116,34 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
return generatedContexts.stream();
}
- void processClusterTemplate(ExtensionContext context, ClusterTemplate
annot,
-
Consumer<TestTemplateInvocationContext> testInvocations) {
+ List<TestTemplateInvocationContext>
processClusterTemplate(ExtensionContext context, ClusterTemplate annot) {
// If specified, call cluster config generated method (must be static)
List<ClusterConfig> generatedClusterConfigs = new ArrayList<>();
+ List<TestTemplateInvocationContext> testTemplateInvocationContexts =
new ArrayList<>();
if (annot.value().trim().isEmpty()) {
throw new IllegalStateException("ClusterTemplate value can't be
empty string.");
}
generateClusterConfigurations(context, annot.value(),
generatedClusterConfigs::add);
- String baseDisplayName = context.getRequiredTestMethod().getName();
- generatedClusterConfigs.forEach(config ->
config.clusterType().invocationContexts(baseDisplayName, config,
testInvocations));
+ if (context.getRequiredTestMethod() != null) {
Review Comment:
We shouldn't need to check for null here. Can you remove the condition?
https://github.com/junit-team/junit5/blob/releases/5.10.x/junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ExtensionContext.java#L236
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -119,23 +116,34 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
return generatedContexts.stream();
}
- void processClusterTemplate(ExtensionContext context, ClusterTemplate
annot,
-
Consumer<TestTemplateInvocationContext> testInvocations) {
+ List<TestTemplateInvocationContext>
processClusterTemplate(ExtensionContext context, ClusterTemplate annot) {
// If specified, call cluster config generated method (must be static)
List<ClusterConfig> generatedClusterConfigs = new ArrayList<>();
+ List<TestTemplateInvocationContext> testTemplateInvocationContexts =
new ArrayList<>();
if (annot.value().trim().isEmpty()) {
throw new IllegalStateException("ClusterTemplate value can't be
empty string.");
}
generateClusterConfigurations(context, annot.value(),
generatedClusterConfigs::add);
- String baseDisplayName = context.getRequiredTestMethod().getName();
- generatedClusterConfigs.forEach(config ->
config.clusterType().invocationContexts(baseDisplayName, config,
testInvocations));
+ if (context.getRequiredTestMethod() != null) {
+ String baseDisplayName = context.getRequiredTestMethod().getName();
+ generatedClusterConfigs.forEach(config ->
config.clusterType().invocationContexts(baseDisplayName, config,
testTemplateInvocationContexts::add));
+ }
+
+ if (testTemplateInvocationContexts.isEmpty()) {
+ throw new IllegalStateException("ClusterConfig generator method
should provide at least one config.");
+ }
+
+ return testTemplateInvocationContexts;
}
private void generateClusterConfigurations(ExtensionContext context,
String generateClustersMethods, ClusterGenerator generator) {
Object testInstance = context.getTestInstance().orElse(null);
- Method method =
ReflectionUtils.getRequiredMethod(context.getRequiredTestClass(),
generateClustersMethods, ClusterGenerator.class);
- ReflectionUtils.invokeMethod(method, testInstance, generator);
+ Class<?> testClass = context.getRequiredTestClass();
+ if (context.getRequiredTestClass() != null) {
Review Comment:
Same here.
https://github.com/junit-team/junit5/blob/releases/5.10.x/junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ExtensionContext.java#L137
Can you remove the `if`?
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensionsUnitTest.java:
##########
@@ -33,16 +31,22 @@ public class ClusterTestExtensionsUnitTest {
void testProcessClusterTemplate() {
ClusterTestExtensions ext = new ClusterTestExtensions();
ExtensionContext context = mock(ExtensionContext.class);
- Consumer<TestTemplateInvocationContext> testInvocations =
mock(Consumer.class);
ClusterTemplate annot = mock(ClusterTemplate.class);
- when(annot.value()).thenReturn("").thenReturn(" ");
+ when(annot.value()).thenReturn("").thenReturn("
").thenReturn("test_empty_config");
Review Comment:
I'm wondering if these tests would be simpler to write with actual methods.
We could define a static inner class:
```java
static class StubTest {
@ClusterTemplate("cfgFoo")
void testFoo() {}
static void cfgFoo(ClusterGenerator gen) { /* ... */ }
@ClusterTemplate("")
void testBar() {}
}
```
and a utility method:
```java
private ExtensionContext buildExtensionContext(String methodName) throws
Exception {
ExtensionContext extensionContext = mock(ExtensionContext.class);
Class clazz = StubTest.class;
Method method = clazz.getDeclaredMethod(methodName);
when(extensionContext.getRequiredTestClass()).thenReturn(clazz);
when(extensionContext.getRequiredTestMethod()).thenReturn(method);
return extensionContext;
}
```
and then we could test it this way:
```java
ClusterTestExtensions clusterTestExtensions = new ClusterTestExtensions();
assertEquals("ClusterConfig generator method should provide at least one
config.",
assertThrows(IllegalStateException.class, () ->
clusterTestExtensions.provideTestTemplateInvocationContexts(buildExtensionContext("testFoo"))
).getMessage()
);
assertEquals("ClusterTemplate value can't be empty string.",
assertThrows(IllegalStateException.class, () ->
clusterTestExtensions.provideTestTemplateInvocationContexts(buildExtensionContext("testBar"))
).getMessage()
);
/* etc */
```
This would also mean that we can keep `processClusterTemplate` private and
test `provideTestTemplateInvocationContexts` directly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
