Skip to content
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.

Sign up for GitHub

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-17587 Refactor test infrastructure #18602

Merged
merged 25 commits into from
Jan 24, 2025
Merged

KAFKA-17587 Refactor test infrastructure #18602

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4046919
everything working
mumrah Jan 17, 2025
a6cf0ec
move things out of test-common
mumrah Jan 17, 2025
d6084ca
remove test-common
mumrah Jan 17, 2025
83f04e6
fix imports. everything compiling again
mumrah Jan 17, 2025
62a2550
rat + spotless
mumrah Jan 17, 2025
4cccb82
fix some errant changes
mumrah Jan 17, 2025
e145452
missing import
mumrah Jan 17, 2025
7bedf6f
fix checkstyle
mumrah Jan 18, 2025
9be040a
fix test-common-runtime
mumrah Jan 18, 2025
be7d777
add some error handling to junit.py
mumrah Jan 18, 2025
914bb46
Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi
mumrah Jan 18, 2025
9e00f04
fix extension
mumrah Jan 18, 2025
1143109
fix test
mumrah Jan 18, 2025
dc00498
Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi
mumrah Jan 18, 2025
f646c98
Fix log4j runtime dep
mumrah Jan 18, 2025
ac5bb0a
rename things
mumrah Jan 18, 2025
cf1cede
utilize @Flaky in clients
mumrah Jan 18, 2025
38f6dd7
Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi
mumrah Jan 18, 2025
25b922a
unused import
mumrah Jan 19, 2025
cf638bc
Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi
mumrah Jan 19, 2025
12696a1
Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi
mumrah Jan 21, 2025
b7d60b7
rename
mumrah Jan 22, 2025
4678853
Merge branch 'trunk-cached' into KAFKA-17587-test-infra-spi
mumrah Jan 22, 2025
a117269
Merge remote-tracking branch 'origin/trunk' into KAFKA-17587-test-inf…
mumrah Jan 23, 2025
d552f59
fixup ShareConsumerTest
mumrah Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/scripts/junit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ class TestSuite:
def clean_test_name(test_name: str) -> str:
cleaned = test_name.strip("\"").rstrip("()")
m = method_matcher.match(cleaned)
return m.group(1)
if m is None:
raise ValueError(f"Could not parse test name '{test_name}'. Expected a valid Java method name.")
else:
return m.group(1)


class TestCatalogExporter:
Expand Down
110 changes: 58 additions & 52 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ ext {
gradleVersion = versions.gradle
minClientJavaVersion = 11
minNonClientJavaVersion = 17
modulesNeedingJava11 = [":clients", ":generator", ":streams", ":streams:test-utils", ":streams-scala", ":test-common:test-common-runtime"]
modulesNeedingJava11 = [":clients", ":generator", ":streams", ":streams:test-utils", ":streams-scala", ":test-common:test-common-util"]

buildVersionFileName = "kafka-version.properties"

Expand Down Expand Up @@ -139,10 +139,11 @@ ext {
runtimeTestLibs = [
libs.slf4jLog4j2,
libs.junitPlatformLanucher,
project(":test-common:test-common-runtime")
libs.jacksonDatabindYaml,
project(":test-common:test-common-util")
]

log4jRuntimeLibs = [
log4jReleaseLibs = [
libs.slf4jLog4j2,
libs.log4j1Bridge2Api,
libs.jacksonDatabindYaml
Expand Down Expand Up @@ -1059,7 +1060,7 @@ project(':core') {
}

dependencies {
releaseOnly log4jRuntimeLibs
releaseOnly log4jReleaseLibs
// `core` is often used in users' tests, define the following dependencies as `api` for backwards compatibility
// even though the `core` module doesn't expose any public API
api project(':clients')
Expand Down Expand Up @@ -1102,8 +1103,9 @@ project(':core') {
testImplementation project(':server-common').sourceSets.test.output
testImplementation project(':storage:storage-api').sourceSets.test.output
testImplementation project(':server').sourceSets.test.output
testImplementation project(':test-common')
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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

testImplementation libs.bcpkix
testImplementation libs.mockitoCore
testImplementation(libs.apacheda) {
Expand Down Expand Up @@ -1364,6 +1366,9 @@ project(':metadata') {
testImplementation project(':raft').sourceSets.test.output
testImplementation project(':server-common').sourceSets.test.output

testImplementation project(':test-common:test-common-internal-api')
testRuntimeOnly project(':test-common:test-common-runtime')
mumrah marked this conversation as resolved.
Show resolved Hide resolved

testRuntimeOnly runtimeTestLibs

generator project(':generator')
Expand Down Expand Up @@ -1535,21 +1540,17 @@ project(':group-coordinator') {
srcJar.dependsOn 'processMessages'
}

project(':test-common') {
// Test framework stuff. Implementations that support test-common-api

project(':test-common:test-common-internal-api') {
// Interfaces, config classes, and other test APIs. Java 17 only
base {
archivesName = "kafka-test-common"
archivesName = "kafka-test-common-internal-api"
}

dependencies {
implementation project(':core')
implementation project(':metadata')
implementation project(':server')
implementation project(':raft')
implementation project(':storage')
implementation project(':server-common')
implementation libs.jacksonDatabindYaml
implementation libs.slf4jApi
implementation project(':server-common') // Only project dependency allowed

implementation libs.junitJupiterApi

testImplementation libs.junitJupiter
testImplementation libs.mockitoCore
Expand All @@ -1559,41 +1560,30 @@ project(':test-common') {
}

checkstyle {
configProperties = checkstyleConfigProperties("import-control-test-common.xml")
configProperties = checkstyleConfigProperties("import-control-test-common-internal-api.xml")
}

javadoc {
enabled = false
}
}

project(':test-common:test-common-api') {
// Interfaces, config classes, and other test APIs
project(':test-common:test-common-util') {
// Runtime-only JUnit extentions for entire project. Java 11 only
mumrah marked this conversation as resolved.
Show resolved Hide resolved
base {
archivesName = "kafka-test-common-api"
archivesName = "kafka-test-common-util"
}

dependencies {
implementation project(':clients')
implementation project(':core')
implementation project(':group-coordinator')
implementation project(':metadata')
implementation project(':raft')
implementation project(':server')
implementation project(':server-common')
implementation project(':storage')
implementation project(':test-common')
implementation libs.junitPlatformLanucher
implementation libs.junitJupiterApi

testImplementation libs.junitJupiter
testImplementation libs.mockitoCore
implementation libs.junitJupiter
implementation libs.slf4jApi
testImplementation testLog4j2Libs

testRuntimeOnly runtimeTestLibs
}

checkstyle {
configProperties = checkstyleConfigProperties("import-control-test-common-api.xml")
configProperties = checkstyleConfigProperties("import-control-test-common-util.xml")
}

javadoc {
Expand All @@ -1602,21 +1592,36 @@ project(':test-common:test-common-api') {
}

project(':test-common:test-common-runtime') {
// Runtime-only test code including JUnit extentions
// Runtime-only JUnit extensions for integration tests. Java 17 only
base {
archivesName = "kafka-test-common-runtime"
}

dependencies {
implementation project(':test-common:test-common-internal-api')
implementation project(':clients')
implementation project(':core')
implementation project(':group-coordinator')
implementation project(':metadata')
implementation project(':raft')
implementation project(':server')
implementation project(':server-common')
implementation project(':storage')

implementation libs.junitPlatformLanucher
implementation libs.junitJupiterApi
implementation libs.junitJupiter
implementation libs.jacksonDatabindYaml
implementation libs.slf4jApi

testImplementation libs.junitJupiter
testImplementation libs.mockitoCore
testImplementation testLog4j2Libs

testRuntimeOnly runtimeTestLibs
}

checkstyle {
configProperties = checkstyleConfigProperties("import-control-test-common-api.xml")
configProperties = checkstyleConfigProperties("import-control-test-common-runtime.xml")
}

javadoc {
Expand Down Expand Up @@ -1644,8 +1649,8 @@ project(':transaction-coordinator') {
testImplementation libs.junitJupiter
testImplementation libs.mockitoCore
testImplementation project(':clients').sourceSets.test.output
testImplementation project(':test-common')
testImplementation project(':test-common:test-common-api')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':test-common:test-common-internal-api')

testRuntimeOnly runtimeTestLibs

Expand Down Expand Up @@ -1865,6 +1870,7 @@ project(':clients') {
compileOnly libs.jose4j // for SASL/OAUTHBEARER JWT validation; only used by broker


testImplementation project(':test-common:test-common-util')
testImplementation libs.bcpkix
testImplementation libs.jacksonJakartarsJsonProvider
testImplementation libs.jose4j
Expand All @@ -1879,7 +1885,6 @@ project(':clients') {
testRuntimeOnly libs.jacksonDatabind
testRuntimeOnly libs.jacksonJDK8Datatypes
testRuntimeOnly runtimeTestLibs
testRuntimeOnly log4jRuntimeLibs

generator project(':generator')
}
Expand Down Expand Up @@ -2266,7 +2271,8 @@ project(':storage') {
testImplementation project(':clients').sourceSets.test.output
testImplementation project(':core')
testImplementation project(':core').sourceSets.test.output
testImplementation project(':test-common:test-common-api')
testImplementation project(':test-common:test-common-internal-api')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':server')
testImplementation project(':server-common')
testImplementation project(':server-common').sourceSets.test.output
Expand Down Expand Up @@ -2423,7 +2429,7 @@ project(':tools') {
}

dependencies {
releaseOnly log4jRuntimeLibs
releaseOnly log4jReleaseLibs

implementation project(':clients')
implementation project(':metadata')
Expand Down Expand Up @@ -2455,7 +2461,8 @@ project(':tools') {
testImplementation project(':server').sourceSets.test.output
testImplementation project(':core')
testImplementation project(':core').sourceSets.test.output
testImplementation project(':test-common:test-common-api')
testImplementation project(':test-common:test-common-internal-api')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':server-common')
testImplementation project(':server-common').sourceSets.test.output
testImplementation project(':connect:api')
Expand All @@ -2466,7 +2473,6 @@ project(':tools') {
testImplementation project(':streams')
testImplementation project(':streams').sourceSets.test.output
testImplementation project(':streams:integration-tests').sourceSets.test.output
testImplementation project(':test-common')
testImplementation libs.junitJupiter
testImplementation libs.mockitoCore
testImplementation libs.mockitoJunitJupiter // supports MockitoExtension
Expand Down Expand Up @@ -2647,7 +2653,6 @@ project(':streams') {

testRuntimeOnly project(':streams:test-utils')
testRuntimeOnly runtimeTestLibs
testRuntimeOnly log4jRuntimeLibs

generator project(':generator')
}
Expand Down Expand Up @@ -2838,7 +2843,7 @@ project(':streams:integration-tests') {
testImplementation project(':storage')
testImplementation project(':streams').sourceSets.test.output
testImplementation project(':streams:streams-scala')
testImplementation project(':test-common')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':tools')
testImplementation project(':transaction-coordinator')
testImplementation libs.bcpkix
Expand Down Expand Up @@ -3514,14 +3519,15 @@ project(':connect:runtime') {
testImplementation project(':server')
testImplementation project(':metadata')
testImplementation project(':server-common')
testImplementation project(':test-common')
testImplementation project(':test-common:test-common-internal-api')
testImplementation project(':test-common:test-common-util')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':server-common')
testImplementation project(':server')
testImplementation project(':group-coordinator')
testImplementation project(':storage')
testImplementation project(':connect:test-plugins')
testImplementation project(':server-common').sourceSets.test.output
testImplementation project(':test-common:test-common-api')

testImplementation libs.jacksonDatabindYaml
testImplementation libs.junitJupiter
Expand Down Expand Up @@ -3635,7 +3641,7 @@ project(':connect:file') {
testImplementation project(':connect:runtime')
testImplementation project(':connect:runtime').sourceSets.test.output
testImplementation project(':core')
testImplementation project(':test-common')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':server-common').sourceSets.test.output

testRuntimeOnly runtimeTestLibs
Expand Down Expand Up @@ -3739,7 +3745,7 @@ project(':connect:mirror') {
testImplementation project(':clients').sourceSets.test.output
testImplementation project(':connect:runtime').sourceSets.test.output
testImplementation project(':core')
testImplementation project(':test-common')
testImplementation project(':test-common:test-common-runtime')
testImplementation project(':server')
testImplementation project(':server-common').sourceSets.test.output

Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-coordinator-common.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
<allow pkg="org.apache.kafka.common.security" />
<allow pkg="org.apache.kafka.common.serialization" />
<allow pkg="org.apache.kafka.common.utils" />
<allow pkg="org.apache.kafka.common.test.api" />

<subpackage name="coordinator">
<subpackage name="common">
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-core.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
<allow pkg="kafka.utils" />
<allow pkg="kafka.serializer" />
<allow pkg="org.apache.kafka.common" />
<allow pkg="org.apache.kafka.common.test.api" />
<allow pkg="org.mockito" class="AssignmentsManagerTest"/>
<allow pkg="org.apache.kafka.server"/>
<allow pkg="org.opentest4j" class="RemoteLogManagerTest"/>
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-group-coordinator.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
<allow pkg="org.apache.kafka.common.utils" />
<allow pkg="org.apache.kafka.common.errors" exact-match="true" />
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.common.test.api" />

<subpackage name="coordinator">
<subpackage name="group">
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-metadata.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
<allow pkg="org.apache.kafka.common.utils" />
<allow pkg="org.apache.kafka.common.errors" exact-match="true" />
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.common.test.api" />

<!-- persistent collection factories/non-library-specific wrappers -->
<allow pkg="org.apache.kafka.server.immutable" exact-match="true" />
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-server-common.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
<allow pkg="org.apache.kafka.common.utils" />
<allow pkg="org.apache.kafka.common.errors" exact-match="true" />
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.common.test.api" />

<!-- persistent collection factories/non-library-specific wrappers -->
<allow pkg="org.apache.kafka.server.immutable" exact-match="true" />
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-server.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.common.network" />
<allow pkg="org.apache.kafka.server.config"/>
<allow pkg="org.apache.kafka.common.test.api" />


<!-- protocol, records and request/response utilities -->
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-share-coordinator.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
<allow pkg="org.apache.kafka.common.security" />
<allow pkg="org.apache.kafka.common.serialization" />
<allow pkg="org.apache.kafka.common.utils" />
<allow pkg="org.apache.kafka.common.test.api" />

<subpackage name="coordinator">
<subpackage name="share">
Expand Down
1 change: 1 addition & 0 deletions checkstyle/import-control-storage.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
<allow pkg="org.apache.kafka.common.errors" exact-match="true" />
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.common.test" />
<allow pkg="org.apache.kafka.common.test.api" />


<subpackage name="server">
Expand Down
Loading
Loading