KAFKA-17587 Refactor test infrastructure #18602

mumrah · 2025-01-17T20:46:32Z

This patch reorganizes our test infrastructure into three Gradle modules:

":test-common:test-common-internal-api" is now a minimal dependency which exposes interfaces and annotations only. It has one project dependency on server-common to expose commonly used data classes (MetadataVersion, Feature, etc). Since this pulls in server-common, this module is Java 17+. It cannot be used by ":clients" or other Java 11 modules.

":test-common:test-common-util" includes the auto-quarantined JUnit extension. The @Flaky annotation has been moved here. Since this module has no project dependencies, we can add it to the Java 11 list so that ":clients" and others can utilize the @Flaky annotation

":test-common:test-common-runtime" now includes all of the test infrastructure code (TestKitNodes, etc). This module carries heavy dependencies (core, etc) and so it should not normally be included as a compile-time dependency.

In addition to this reorganization, this patch leverages JUnit SPI service discovery so that modules can utilize the integration test framework without depending on ":core". This will allow us to start moving integration tests out of core and into the appropriate sub-module. This is done by adding ":test-common:test-common-runtime" as a testRuntimeOnly dependency rather than as a testImplementation dependency. A trivial example was added to QuorumControllerTest to illustrate this.

This patch does not expose ClusterInstance through the SPI since it has many direct dependencies on core. A stripped-down version of ClusterInstance that can be used by non-core modules remains as future work.

chia7712

@mumrah thanks for this refactor. I run the command ./gradlew cleanTest test-common:test-common-runtime:test and it results in some error below:

Gradle Test Run :test-common:test-common-runtime:test > Gradle Test Executor 109 > TestKitNodeTest > testSecurityProtocol(SecurityProtocol) > initializationError FAILED
    java.lang.IllegalStateException: Please annotate test methods with @ClusterTemplate, @ClusterTest, or @ClusterTests when using the ClusterTestExtensions provider
        at org.apache.kafka.common.test.junit.ClusterTestExtensions.provideTestTemplateInvocationContexts(ClusterTestExtensions.java:147)
        at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:273)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

build.gradle

chia7712 · 2025-01-18T03:12:23Z

test-common/test-common-api/src/main/java/org/apache/kafka/common/test/api/README.md

@@ -79,7 +79,7 @@ a JUnit extension called `ClusterTestExtensions` which knows how to process thes
 invocations. Test classes that wish to make use of these annotations need to explicitly register this extension:

 ```scala
-import org.apache.kafka.common.test.api.ClusterTestExtensions
+import org.apache.kafka.common.test.junit.ClusterTestExtensions

 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))


we don't need it, right?

Correct. I'll update these docs.

chia7712 · 2025-01-18T03:13:03Z

core/src/test/scala/unit/kafka/server/SaslApiVersionsRequestTest.scala

 import org.apache.kafka.common.config.SaslConfigs
 import org.apache.kafka.common.config.internals.BrokerSecurityConfigs
 import org.apache.kafka.common.message.SaslHandshakeRequestData
 import org.apache.kafka.common.protocol.{ApiKeys, Errors}
 import org.apache.kafka.common.requests.{ApiVersionsRequest, ApiVersionsResponse, SaslHandshakeRequest, SaslHandshakeResponse}
 import org.apache.kafka.common.security.auth.SecurityProtocol
+import org.apache.kafka.common.test.ClusterInstance
+import org.apache.kafka.common.test.junit.ClusterTestExtensions


Should we remove @ExtendWith(value = Array(classOf[ClusterTestExtensions])) from this class?

Wow, good catch 😄 I left this one here intentionally to make sure it still worked if people explicitly add the annotation in the future

Well this was a bad choice for a test since this one is disabled. I tried it locally on another test and verified keeping the annotation is harmless

mumrah · 2025-01-18T04:38:01Z

@chia7712 I think this commit should fix the initialization issue. Since SPI is being used, it loads the extension for all tests. We need some additional filtering to not apply the ClusterTest logic to regular tests.

ijuma · 2025-01-18T13:38:25Z

Thanks for the PR. A couple of questions:

Do we intend to have a common test module for clients and streams (aside from junit specific classes)?
If we were to provide a test module for external usage, what would we call it?

These questions will help validate the names we have chosen.

mumrah · 2025-01-18T14:18:05Z

@ijuma,

Do we intend to have a common test module for clients and streams (aside from junit specific classes)?

I think that would make sense. There's quite a bit of utility test code that could be kept at Java 11. In this case we might consider test-common-util instead of test-common-junit

If we were to provide a test module for external usage, what would we call it?

I was thinking testkit or test-kit (so kafka-testkit.jar or kafka-test-kit.jar). Are you thinking we go ahead and establish a public-friendly name for the test-common-api module?

ijuma · 2025-01-18T15:02:25Z

Makes sense to use the testkit name for the external dependency and going with util for a more general module name that can include shared junit stuff but also other useful things needed by all test modules.

The only open question for me is the test-common-api name. Even though it makes sense, we typically use api for modules that expose apis externally (group-coordinator-api, storage-api, tools-api, etc.). In this case, it's a purely internal module, so I wonder if we can find a different name to describe it.

mumrah · 2025-01-18T15:17:04Z

Hm, yea.. I see what you mean. Here are some ideas for alternatives to test-common-api

test-common-private-api
test-common-defs
test-common-lib
test-common-spi (sort of a misnomer)
test-common

ijuma · 2025-01-18T16:09:00Z

Since you're calling one test-common-runtime, would test-common-compile be appropriate? Or perhaps test-common-internal-api (we already use internals in our package names). The latter could be used for any module that exposes internal apis, so perhaps it a reasonable pattern to establish.

mumrah · 2025-01-18T16:42:06Z

I like "internal-api" 👍

build.gradle

...common/test-common-internal-api/src/main/java/org/apache/kafka/common/test/api/Defaults.java

chia7712 · 2025-01-19T13:18:34Z

build.gradle

-    testImplementation project(':test-common:test-common-api')
+    testImplementation project(':test-common:test-common-runtime')
+    testImplementation project(':test-common:test-common-internal-api')
+    testImplementation project(':test-common:test-common-util')


It seems clients, core, and connect:runtime need this because they need to import Flaky. Maybe we can move Flaky to test-common:test-common-internal-api? test-common:test-common-internal-api can be under java 11 if we move server-common out of test-common:test-common-internal-api. test-common:test-common-internal-api is using enum classes of server-common so they can be replaced by string easily. With that change, we can move AutoQuarantinedTestFilter and QuarantinedPostDiscoveryFilter to test-common:test-common-runtime to remove the test-common:test-common-util also

Let me see if I understand your proposal

Refactor "test-common-internal-api" to not depend on ":server-common"

Move "test-common-internal-api" to Java 11

Move @Flaky to the "test-common-internal-api"

Remove "test-common-util"

I think this would make sense, except that Java 11 modules like ":clients" cannot bring a runtime dependency with a newer java version. I tried this and got:

* What went wrong: Could not determine the dependencies of task ':clients:test'. > Could not resolve all dependencies for configuration ':clients:testRuntimeClasspath'. > Could not resolve project :test-common:test-common-runtime. Required by: project :clients > Dependency resolution is looking for a library compatible with JVM runtime version 11, but 'project :test-common:test-common-runtime' is only compatible with JVM runtime version 17 or newer.

Do you know of a way around this?

Since test-common-internal-api is meant to be the abstraction for the test infrastructure offered by test-common-runtime, it doesn't make sense to include one without the other. It might be confusing for developers if @ClusterTest was available to import, but was not actually usable.

If we want the client integration tests to be moved out of core, we should create a ":clients:integration-tests" module that is Java 17 and move the tests there (similar to what streams does).

I expect "test-common-util" will grow to include helper/common test methods. We can probably combine the most of the various TestUtils into a single Java 11 utils class under this module.

Since test-common-internal-api is meant to be the abstraction for the test infrastructure offered by test-common-runtime, it doesn't make sense to include one without the other. It might be confusing for developers if @clustertest was available to import, but was not actually usable.

Yes, you're right. Let's keep the current structure.

mumrah · 2025-01-22T19:44:00Z

@ijuma @chia7712 did you have any further comments on this PR?

chia7712 · 2025-01-23T17:30:21Z

@mumrah could you please fix the conflicts? Additionally, should we backport it to 4.0?

…ra-spi

mumrah · 2025-01-23T20:00:28Z

@chia7712 yea I can backport it. That will make it easier to backport test changes during the ongoing 4.0.0 release and down the road in 4.0.x

chia7712

LGTM

@flaky

This patch reorganizes our test infrastructure into three Gradle modules: ":test-common:test-common-internal-api" is now a minimal dependency which exposes interfaces and annotations only. It has one project dependency on server-common to expose commonly used data classes (MetadataVersion, Feature, etc). Since this pulls in server-common, this module is Java 17+. It cannot be used by ":clients" or other Java 11 modules. ":test-common:test-common-util" includes the auto-quarantined JUnit extension. The @flaky annotation has been moved here. Since this module has no project dependencies, we can add it to the Java 11 list so that ":clients" and others can utilize the @flaky annotation ":test-common:test-common-runtime" now includes all of the test infrastructure code (TestKitNodes, etc). This module carries heavy dependencies (core, etc) and so it should not normally be included as a compile-time dependency. In addition to this reorganization, this patch leverages JUnit SPI service discovery so that modules can utilize the integration test framework without depending on ":core". This will allow us to start moving integration tests out of core and into the appropriate sub-module. This is done by adding ":test-common:test-common-runtime" as a testRuntimeOnly dependency rather than as a testImplementation dependency. A trivial example was added to QuorumControllerTest to illustrate this. Reviewers: Ismael Juma <[email protected]>, Chia-Ping Tsai <[email protected]>

mumrah added 4 commits January 17, 2025 14:53

everything working

4046919

move things out of test-common

a6cf0ec

remove test-common

d6084ca

fix imports. everything compiling again

83f04e6

github-actions bot added core Kafka Broker tools kraft build Gradle build or GitHub Actions labels Jan 17, 2025

mumrah added 4 commits January 17, 2025 15:52

rat + spotless

62a2550

fix some errant changes

4cccb82

missing import

e145452

fix checkstyle

7bedf6f

github-actions bot added storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature labels Jan 18, 2025

mumrah added 3 commits January 17, 2025 21:25

fix test-common-runtime

9be040a

add some error handling to junit.py

be7d777

Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi

914bb46

chia7712 reviewed Jan 18, 2025

View reviewed changes

fix extension

9e00f04

mumrah added 3 commits January 18, 2025 09:29

fix test

1143109

Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi

dc00498

Fix log4j runtime dep

f646c98

rename things

ac5bb0a

mumrah added 2 commits January 18, 2025 16:09

utilize @flaky in clients

cf1cede

Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi

38f6dd7

github-actions bot added consumer clients labels Jan 18, 2025

mumrah added 2 commits January 18, 2025 23:15

unused import

25b922a

Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi

cf638bc

chia7712 reviewed Jan 19, 2025

View reviewed changes

mumrah added 3 commits January 21, 2025 09:31

Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi

12696a1

rename

b7d60b7

Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi

4678853

Merge remote-tracking branch 'origin/trunk' into KAFKA-17587-test-inf…

a117269

…ra-spi

fixup ShareConsumerTest

d552f59

mumrah requested a review from chia7712 January 23, 2025 21:01

mumrah mentioned this pull request Jan 23, 2025

KAFKA-18617 Allow use of ClusterInstance inside BeforeEach #18662

Open

chia7712 approved these changes Jan 24, 2025

View reviewed changes

mumrah merged commit 8c0a0e0 into apache:trunk Jan 24, 2025
8 of 9 checks passed

mumrah mentioned this pull request Jan 24, 2025

KAFKA-17587 Refactor test infrastructure (4.0) #18694

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17587 Refactor test infrastructure #18602

KAFKA-17587 Refactor test infrastructure #18602

mumrah commented Jan 17, 2025 •

edited

Loading

chia7712 left a comment

chia7712 Jan 18, 2025

mumrah Jan 18, 2025

chia7712 Jan 18, 2025

mumrah Jan 18, 2025

mumrah Jan 18, 2025

mumrah commented Jan 18, 2025

ijuma commented Jan 18, 2025

mumrah commented Jan 18, 2025 •

edited

Loading

ijuma commented Jan 18, 2025

mumrah commented Jan 18, 2025

ijuma commented Jan 18, 2025 •

edited

Loading

mumrah commented Jan 18, 2025

chia7712 Jan 19, 2025

mumrah Jan 21, 2025

chia7712 Jan 21, 2025

mumrah commented Jan 22, 2025

chia7712 commented Jan 23, 2025

mumrah commented Jan 23, 2025

chia7712 left a comment

KAFKA-17587 Refactor test infrastructure #18602

KAFKA-17587 Refactor test infrastructure #18602

Conversation

mumrah commented Jan 17, 2025 • edited Loading

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumrah commented Jan 18, 2025

ijuma commented Jan 18, 2025

mumrah commented Jan 18, 2025 • edited Loading

ijuma commented Jan 18, 2025

mumrah commented Jan 18, 2025

ijuma commented Jan 18, 2025 • edited Loading

mumrah commented Jan 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumrah commented Jan 22, 2025

chia7712 commented Jan 23, 2025

mumrah commented Jan 23, 2025

chia7712 left a comment

Choose a reason for hiding this comment

mumrah commented Jan 17, 2025 •

edited

Loading

mumrah commented Jan 18, 2025 •

edited

Loading

ijuma commented Jan 18, 2025 •

edited

Loading