chia7712 commented on code in PR #15916:
URL: https://github.com/apache/kafka/pull/15916#discussion_r1603194691
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensionsUnitTest.java:
##########
@@ -33,16 +31,16 @@ public class ClusterTestExtensionsUnitTest {
void testProcessClusterTemplate() {
Review Comment:
`@SuppressWarnings("unchecked")` is unused now
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -94,59 +93,80 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
// Process the @ClusterTemplate annotation
ClusterTemplate clusterTemplateAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTemplate.class);
if (clusterTemplateAnnot != null) {
- processClusterTemplate(context, clusterTemplateAnnot,
generatedContexts::add);
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("ClusterConfig generator
method should provide at least one config");
- }
+ generatedContexts.addAll(processClusterTemplate(context,
clusterTemplateAnnot));
}
// Process single @ClusterTest annotation
ClusterTest clusterTestAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTest.class);
if (clusterTestAnnot != null) {
- processClusterTest(context, clusterTestAnnot, defaults,
generatedContexts::add);
+ generatedContexts.addAll(processClusterTest(context,
clusterTestAnnot, defaults));
}
// Process multiple @ClusterTest annotation within @ClusterTests
ClusterTests clusterTestsAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTests.class);
if (clusterTestsAnnot != null) {
- for (ClusterTest annot : clusterTestsAnnot.value()) {
- processClusterTest(context, annot, defaults,
generatedContexts::add);
- }
- }
-
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("Please annotate test methods with
@ClusterTemplate, @ClusterTest, or " +
- "@ClusterTests when using the ClusterTestExtensions
provider");
+ generatedContexts.addAll(processClusterTests(context,
clusterTestsAnnot, defaults));
}
return generatedContexts.stream();
}
- void processClusterTemplate(ExtensionContext context, ClusterTemplate
annot,
Review Comment:
We can simplify the code by lambda. For example:
```java
List<TestTemplateInvocationContext>
processClusterTemplate(ExtensionContext context, ClusterTemplate annot) {
if (annot.value().trim().isEmpty()) throw new
IllegalStateException("ClusterTemplate value can't be empty string.");
String baseDisplayName = context.getRequiredTestMethod().getName();
List<TestTemplateInvocationContext> contexts =
generateClusterConfigurations(context, annot.value())
.stream().flatMap(config -> config.clusterTypes().stream()
.map(type -> type.invocationContexts(baseDisplayName,
config))).collect(Collectors.toList());
if (contexts.isEmpty()) throw new
IllegalStateException("ClusterConfig generator method should provide at least
one config");
return contexts;
}
```
##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -62,14 +64,16 @@ public class ClusterTestExtensionsTest {
}
// Static methods can generate cluster configurations
- static void generate1(ClusterGenerator clusterGenerator) {
+ static List<ClusterConfig> generate1() {
+ List<ClusterConfig> ret = new ArrayList<>();
Map<String, String> serverProperties = new HashMap<>();
serverProperties.put("foo", "bar");
- clusterGenerator.accept(ClusterConfig.defaultBuilder()
+ ret.add(ClusterConfig.defaultBuilder()
.setTypes(Collections.singleton(Type.ZK))
.setServerProperties(serverProperties)
.setTags(Collections.singletonList("Generated Test"))
.build());
+ return ret;
Review Comment:
`return Arrays.asList(classicGroupCoordinator, consumerGroupCoordinator);`
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -94,59 +93,80 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
// Process the @ClusterTemplate annotation
ClusterTemplate clusterTemplateAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTemplate.class);
if (clusterTemplateAnnot != null) {
- processClusterTemplate(context, clusterTemplateAnnot,
generatedContexts::add);
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("ClusterConfig generator
method should provide at least one config");
- }
+ generatedContexts.addAll(processClusterTemplate(context,
clusterTemplateAnnot));
}
// Process single @ClusterTest annotation
ClusterTest clusterTestAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTest.class);
if (clusterTestAnnot != null) {
- processClusterTest(context, clusterTestAnnot, defaults,
generatedContexts::add);
+ generatedContexts.addAll(processClusterTest(context,
clusterTestAnnot, defaults));
}
// Process multiple @ClusterTest annotation within @ClusterTests
ClusterTests clusterTestsAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTests.class);
if (clusterTestsAnnot != null) {
- for (ClusterTest annot : clusterTestsAnnot.value()) {
- processClusterTest(context, annot, defaults,
generatedContexts::add);
- }
- }
-
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("Please annotate test methods with
@ClusterTemplate, @ClusterTest, or " +
- "@ClusterTests when using the ClusterTestExtensions
provider");
+ generatedContexts.addAll(processClusterTests(context,
clusterTestsAnnot, defaults));
}
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> contexts = new ArrayList<>();
+
if (annot.value().trim().isEmpty()) {
throw new IllegalStateException("ClusterTemplate value can't be
empty string.");
}
- generateClusterConfigurations(context, annot.value(),
generatedClusterConfigs::add);
+ generatedClusterConfigs.addAll(generateClusterConfigurations(context,
annot.value()));
String baseDisplayName = context.getRequiredTestMethod().getName();
generatedClusterConfigs.forEach(config -> {
for (Type type: config.clusterTypes()) {
- type.invocationContexts(baseDisplayName, config,
testInvocations);
+ contexts.add(type.invocationContexts(baseDisplayName, config));
}
});
+
+ if (contexts.isEmpty()) {
+ throw new IllegalStateException("ClusterConfig generator method
should provide at least one config");
+ }
+
+ return contexts;
}
- private void generateClusterConfigurations(ExtensionContext context,
String generateClustersMethods, ClusterGenerator generator) {
+ private List<ClusterConfig> generateClusterConfigurations(ExtensionContext
context, String generateClustersMethods) {
Object testInstance = context.getTestInstance().orElse(null);
- Method method =
ReflectionUtils.getRequiredMethod(context.getRequiredTestClass(),
generateClustersMethods, ClusterGenerator.class);
- ReflectionUtils.invokeMethod(method, testInstance, generator);
+ Method method =
ReflectionUtils.getRequiredMethod(context.getRequiredTestClass(),
generateClustersMethods);
+ return (List<ClusterConfig>) ReflectionUtils.invokeMethod(method,
testInstance);
+ }
+
+ private List<TestTemplateInvocationContext>
processClusterTests(ExtensionContext context, ClusterTests annots,
ClusterTestDefaults defaults) {
Review Comment:
ditto
```java
private List<TestTemplateInvocationContext>
processClusterTests(ExtensionContext context, ClusterTests annots,
ClusterTestDefaults defaults) {
List<TestTemplateInvocationContext> ret =
Arrays.stream(annots.value())
.flatMap(annot -> processClusterTestInternal(context, annot,
defaults).stream()).collect(Collectors.toList());
if (ret.isEmpty()) throw new
IllegalStateException("processClusterTests method should provide at least one
config");
return ret;
}
```
##########
tools/src/test/java/org/apache/kafka/tools/consumer/group/DeleteConsumerGroupsTest.java:
##########
@@ -35,12 +36,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
+import java.util.*;
Review Comment:
please don't use `*`. that is not allowed in kafka
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -169,9 +189,12 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
.setMetadataVersion(annot.metadataVersion())
.setTags(Arrays.asList(annot.tags()))
.build();
+
+ List<TestTemplateInvocationContext> ret = new ArrayList<>();
for (Type type : types) {
- type.invocationContexts(context.getRequiredTestMethod().getName(),
config, testInvocations);
+
ret.add(type.invocationContexts(context.getRequiredTestMethod().getName(),
config));
}
+ return ret;
Review Comment:
ditto
```java
return Arrays.stream(types).map(type ->
type.invocationContexts(context.getRequiredTestMethod().getName(), config))
.collect(Collectors.toList());
```
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -94,59 +93,80 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
// Process the @ClusterTemplate annotation
ClusterTemplate clusterTemplateAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTemplate.class);
if (clusterTemplateAnnot != null) {
- processClusterTemplate(context, clusterTemplateAnnot,
generatedContexts::add);
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("ClusterConfig generator
method should provide at least one config");
- }
+ generatedContexts.addAll(processClusterTemplate(context,
clusterTemplateAnnot));
}
// Process single @ClusterTest annotation
ClusterTest clusterTestAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTest.class);
if (clusterTestAnnot != null) {
- processClusterTest(context, clusterTestAnnot, defaults,
generatedContexts::add);
+ generatedContexts.addAll(processClusterTest(context,
clusterTestAnnot, defaults));
}
// Process multiple @ClusterTest annotation within @ClusterTests
ClusterTests clusterTestsAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTests.class);
if (clusterTestsAnnot != null) {
- for (ClusterTest annot : clusterTestsAnnot.value()) {
- processClusterTest(context, annot, defaults,
generatedContexts::add);
- }
- }
-
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("Please annotate test methods with
@ClusterTemplate, @ClusterTest, or " +
- "@ClusterTests when using the ClusterTestExtensions
provider");
+ generatedContexts.addAll(processClusterTests(context,
clusterTestsAnnot, defaults));
}
return generatedContexts.stream();
Review Comment:
Could you add a final check for `generatedContexts`? it should not be empty
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -94,59 +93,80 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
// Process the @ClusterTemplate annotation
ClusterTemplate clusterTemplateAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTemplate.class);
if (clusterTemplateAnnot != null) {
- processClusterTemplate(context, clusterTemplateAnnot,
generatedContexts::add);
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("ClusterConfig generator
method should provide at least one config");
- }
+ generatedContexts.addAll(processClusterTemplate(context,
clusterTemplateAnnot));
}
// Process single @ClusterTest annotation
ClusterTest clusterTestAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTest.class);
if (clusterTestAnnot != null) {
- processClusterTest(context, clusterTestAnnot, defaults,
generatedContexts::add);
+ generatedContexts.addAll(processClusterTest(context,
clusterTestAnnot, defaults));
}
// Process multiple @ClusterTest annotation within @ClusterTests
ClusterTests clusterTestsAnnot =
context.getRequiredTestMethod().getDeclaredAnnotation(ClusterTests.class);
if (clusterTestsAnnot != null) {
- for (ClusterTest annot : clusterTestsAnnot.value()) {
- processClusterTest(context, annot, defaults,
generatedContexts::add);
- }
- }
-
- if (generatedContexts.isEmpty()) {
- throw new IllegalStateException("Please annotate test methods with
@ClusterTemplate, @ClusterTest, or " +
- "@ClusterTests when using the ClusterTestExtensions
provider");
+ generatedContexts.addAll(processClusterTests(context,
clusterTestsAnnot, defaults));
}
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> contexts = new ArrayList<>();
+
if (annot.value().trim().isEmpty()) {
throw new IllegalStateException("ClusterTemplate value can't be
empty string.");
}
- generateClusterConfigurations(context, annot.value(),
generatedClusterConfigs::add);
+ generatedClusterConfigs.addAll(generateClusterConfigurations(context,
annot.value()));
String baseDisplayName = context.getRequiredTestMethod().getName();
generatedClusterConfigs.forEach(config -> {
for (Type type: config.clusterTypes()) {
- type.invocationContexts(baseDisplayName, config,
testInvocations);
+ contexts.add(type.invocationContexts(baseDisplayName, config));
}
});
+
+ if (contexts.isEmpty()) {
+ throw new IllegalStateException("ClusterConfig generator method
should provide at least one config");
+ }
+
+ return contexts;
}
- private void generateClusterConfigurations(ExtensionContext context,
String generateClustersMethods, ClusterGenerator generator) {
Review Comment:
please add `@SuppressWarnings("unchecked")`
--
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]
