You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "soarez (via GitHub)" <gi...@apache.org> on 2026/05/02 08:53:41 UTC
[PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
soarez opened a new pull request, #22193:
URL: https://github.com/apache/kafka/pull/22193
This change moves TestSslUtils, TestUtils and accessory classes from the test classpath in `clients` to a new module: `test-common-base`, allowing other modules to use these classes for testing without having to include all of the test classpath for `clients`.
The package name for these classes changed, from `org.apache.kafka.test` to `org.apache.kafka.base.test`.
The move from a test classpath to a main classpath triggered some issues with spotbugs which were addressed. A couple of reflection utilities required a new exception, and were moved to their own class to isolate it.
For some modules it was possible to replace the dependency on `project(':clients').sourceSets.test.output` with `project(:test-common:test-common-base)`, but the others still have to keep both dependencies for now.
For further details on the context and purpose of this change, check this thread:
https://github.com/apache/kafka/pull/20376#discussion_r3167281569
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on PR #22193:
URL: https://github.com/apache/kafka/pull/22193#issuecomment-4386302361
closing this since we merged #22201
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3177830050
##########
checkstyle/import-control-jmh-benchmarks.xml:
##########
@@ -54,9 +54,10 @@
<allow pkg="org.apache.kafka.coordinator.transaction"/>
<allow pkg="org.apache.kafka.connect" />
<allow pkg="org.apache.kafka.network" />
+ <allow pkg="org.apache.kafka.base.test" />
<allow class="org.apache.kafka.raft.QuorumConfig"/>
<allow class="org.apache.kafka.raft.KRaftConfigs"/>
- <allow class="org.apache.kafka.test.TestUtils"/>
+ <allow class="org.apache.kafka.nase.test.TestUtils"/>
Review Comment:
yes, good catch. removed this rule
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3176818959
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/clients/admin/DeleteTopicTest.java:
##########
@@ -18,6 +18,7 @@
import kafka.server.KafkaBroker;
+import org.apache.kafka.base.test.TestUtils;
Review Comment:
In order to align with other modules, maybe we should use `org.apache.kafka.common.test.base` instead
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on PR #22193:
URL: https://github.com/apache/kafka/pull/22193#issuecomment-4363669534
@m1a2st where would be a good place for docs? We can add a README.md, but I haven't found one for other modules. Currently there's only a few lines of comment in build.gradle, like others, to explain the purpose of this module.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on PR #22193:
URL: https://github.com/apache/kafka/pull/22193#issuecomment-4366153266
Thanks @soarez,I think we can address these in a follow-up patch.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3178891141
##########
build.gradle:
##########
@@ -1574,6 +1578,32 @@ project(':group-coordinator') {
srcJar.dependsOn 'processMessages'
}
+project(':test-common:test-common-base') {
+ // Generic utilities for testing. Depends on `clients` only to minimize classpath leakage.
+ // e.g. SSL utilities, wait functions, file-related operations
+ // Java 11 is the minimum Java version required.
+
+ base {
+ archivesName = "kafka-test-common-base"
+ }
+
+ dependencies {
+ implementation project(':clients')
Review Comment:
This reference causes AppInfoParserTest to fail, as it makes the test scope depend on the JAR rather than class files. While not a serious issue, it does prompt me to rethink the approach of this PR. Perhaps we should consider using fixtures that allow us to keep the same package while maintaining isolation. WDYT?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3176823564
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/clients/admin/ConcurrentListOffsetsRequestTest.java:
##########
@@ -16,6 +16,7 @@
*/
package org.apache.kafka.clients.admin;
+import org.apache.kafka.base.test.ReflectionUtils;
Review Comment:
Have you considered using `TestReflectionUtils`?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3177831000
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/clients/admin/ConcurrentListOffsetsRequestTest.java:
##########
@@ -16,6 +16,7 @@
*/
package org.apache.kafka.clients.admin;
+import org.apache.kafka.base.test.ReflectionUtils;
Review Comment:
I didn't, but I like it. The 'Test' prefix makes it clear this is a test utility. Renamed
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3181245475
##########
build.gradle:
##########
@@ -1574,6 +1578,32 @@ project(':group-coordinator') {
srcJar.dependsOn 'processMessages'
}
+project(':test-common:test-common-base') {
+ // Generic utilities for testing. Depends on `clients` only to minimize classpath leakage.
+ // e.g. SSL utilities, wait functions, file-related operations
+ // Java 11 is the minimum Java version required.
+
+ base {
+ archivesName = "kafka-test-common-base"
+ }
+
+ dependencies {
+ implementation project(':clients')
Review Comment:
This was creating a circular dependency, which I think by itself is undesirable. The AppInfoParserTest issue came from this making tests depend on the clients shadowjar which runs `createVersionFile`. The problem can be fixed by replacing the dependency with the raw main classpath,, done in [2bbdd9e](https://github.com/apache/kafka/pull/22193/commits/2bbdd9eeb65ddd0ff5646088cd2919173182e5a4). But I agree: it begs the question if this is the right approach.
I gave test fixtures approach a shot in #22201. This approach also creates challenges: shadowjar and test-fixtures do not play well together, and we have to sort of corrupt the idea of test fixtures is meant to be to make it work. But overall, I think it is still a cleaner and simpler approach than introducing this new test-common-base module. So if you agree my current inclination would be to close this PR and merge #22201.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez closed pull request #22193: MINOR: extract TestSslUtils to new test-common-base
URL: https://github.com/apache/kafka/pull/22193
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on PR #22193:
URL: https://github.com/apache/kafka/pull/22193#issuecomment-4365962160
> Should [JaasUtils](https://github.com/apache/kafka/blob/trunk/test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/JaasUtils.java) also be moved to this new module?
@m1a2st I can't see a reason why not. These are the implications:
* `test-common-runtime` would have to depend on `test-common-base`.
* `JaasModule` would have to move too.
* Both classes would need a rewrite to avoid the use of `record` due to the language level requirement for test-common-base.
If we all think the above are acceptable changes, I'm happy to include this in a subsequent patch, as this one is already quite large and changes a lot of import statements, making it easy to run into merge conflicts.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3177830420
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/clients/admin/DeleteTopicTest.java:
##########
@@ -18,6 +18,7 @@
import kafka.server.KafkaBroker;
+import org.apache.kafka.base.test.TestUtils;
Review Comment:
Agreed. I renamed the package
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on PR #22193:
URL: https://github.com/apache/kafka/pull/22193#issuecomment-4363714759
I think we can place the `README.md` in the root of the module, similar to https://github.com/apache/kafka/tree/trunk/trogdor
I’m wondering if we could add more comprehensive documentation to clarify what this module is meant to contain.
For now, a comment in build.gradle might be sufficient.
> SSL utilities, wait functions, file-related operations
Should [JaasUtils](https://github.com/apache/kafka/blob/trunk/test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/JaasUtils.java) also be moved to this new module?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3176815922
##########
checkstyle/import-control-jmh-benchmarks.xml:
##########
@@ -54,9 +54,10 @@
<allow pkg="org.apache.kafka.coordinator.transaction"/>
<allow pkg="org.apache.kafka.connect" />
<allow pkg="org.apache.kafka.network" />
+ <allow pkg="org.apache.kafka.base.test" />
<allow class="org.apache.kafka.raft.QuorumConfig"/>
<allow class="org.apache.kafka.raft.KRaftConfigs"/>
- <allow class="org.apache.kafka.test.TestUtils"/>
+ <allow class="org.apache.kafka.nase.test.TestUtils"/>
Review Comment:
typo? `nase`
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: extract TestSslUtils to new test-common-base [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #22193:
URL: https://github.com/apache/kafka/pull/22193#discussion_r3180620133
##########
build.gradle:
##########
@@ -1574,6 +1578,32 @@ project(':group-coordinator') {
srcJar.dependsOn 'processMessages'
}
+project(':test-common:test-common-base') {
+ // Generic utilities for testing. Depends on `clients` only to minimize classpath leakage.
+ // e.g. SSL utilities, wait functions, file-related operations
+ // Java 11 is the minimum Java version required.
+
+ base {
+ archivesName = "kafka-test-common-base"
+ }
+
+ dependencies {
+ implementation project(':clients')
Review Comment:
I noticed that you are using `implementation project(':clients').sourceSets.main.output` instead of fixtures. Would you mind sharing reasoning behind this?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org