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 all 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
107 changes: 55 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 @@ -1535,21 +1537,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 +1557,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 extensions for entire project. Java 11 only
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 +1589,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 +1646,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 @@ -1866,6 +1868,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 @@ -1880,7 +1883,6 @@ project(':clients') {
testRuntimeOnly libs.jacksonDatabind
testRuntimeOnly libs.jacksonJDK8Datatypes
testRuntimeOnly runtimeTestLibs
testRuntimeOnly log4jRuntimeLibs

generator project(':generator')
}
Expand Down Expand Up @@ -2267,7 +2269,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 @@ -2424,7 +2427,7 @@ project(':tools') {
}

dependencies {
releaseOnly log4jRuntimeLibs
releaseOnly log4jReleaseLibs

implementation project(':clients')
implementation project(':metadata')
Expand Down Expand Up @@ -2456,7 +2459,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 @@ -2467,7 +2471,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 @@ -2648,7 +2651,6 @@ project(':streams') {

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

generator project(':generator')
}
Expand Down Expand Up @@ -2839,7 +2841,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 @@ -3515,14 +3517,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 @@ -3636,7 +3639,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 @@ -3740,7 +3743,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
43 changes: 43 additions & 0 deletions checkstyle/import-control-test-common-internal-api.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<!DOCTYPE import-control PUBLIC
"-//Puppy Crawl//DTD Import Control 1.1//EN"
"http://www.puppycrawl.com/dtds/import_control_1_1.dtd">
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->

<import-control pkg="org.apache.kafka">
<!-- no one depends on the server -->
<disallow pkg="kafka" />

<!-- anyone can use public classes -->
<allow pkg="org.apache.kafka.common" exact-match="true" />
<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.errors" exact-match="true" />
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.common.network" />
<allow pkg="org.apache.kafka.common.test" />

<!-- things from server-common -->
<allow pkg="org.apache.kafka.server.common" />

<allow pkg="java" />
<allow pkg="javax.security" />
<allow pkg="org.junit" />
<allow pkg="org.slf4j" />

</import-control>
Loading
Loading