MINOR: extract TestSslUtils to new test-common-base#22193
MINOR: extract TestSslUtils to new test-common-base#22193soarez wants to merge 11 commits intoapache:trunkfrom
Conversation
|
@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. |
|
I think we can place the I’m wondering if we could add more comprehensive documentation to clarify what this module is intended to contain. There are already many different TestUtils in all module, and without clear guidelines, it’s easy for the utilities in to For now, a comment in build.gradle might be sufficient.
Should JaasUtils also be moved to this new module? |
| <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"/> |
There was a problem hiding this comment.
yes, good catch. removed this rule
|
|
||
| import kafka.server.KafkaBroker; | ||
|
|
||
| import org.apache.kafka.base.test.TestUtils; |
There was a problem hiding this comment.
In order to align with other modules, maybe we should use org.apache.kafka.common.test.base instead
There was a problem hiding this comment.
Agreed. I renamed the package
| */ | ||
| package org.apache.kafka.clients.admin; | ||
|
|
||
| import org.apache.kafka.base.test.ReflectionUtils; |
There was a problem hiding this comment.
Have you considered using TestReflectionUtils?
There was a problem hiding this comment.
I didn't, but I like it. The 'Test' prefix makes it clear this is a test utility. Renamed
@m1a2st I can't see a reason why not. These are the implications:
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. |
|
Thanks @soarez, I think we can address these in a follow-up patch. |
| } | ||
|
|
||
| dependencies { | ||
| implementation project(':clients') |
There was a problem hiding this 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?
There was a problem hiding this comment.
I noticed that you are using implementation project(':clients').sourceSets.main.output instead of fixtures. Would you mind sharing reasoning behind this?
This change moves TestSslUtils, TestUtils and accessory classes from the
test classpath in
clientsto 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.testto
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.outputwithproject(:test-common:test-common-base), but the others still have tokeep both dependencies for now.
For further details on the context and purpose of this change, check
this thread:
#20376 (comment)
Reviewers: Ken Huang s7133700@gmail.com, Chia-Ping Tsai
chia7712@gmail.com