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.

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

MINOR Consolidate junit-platform.properties #17399

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

mumrah
Copy link
Member

@mumrah mumrah commented Oct 7, 2024

Now that we have ":test-common" we can fix the underlying problem from KAFKA-17121. We should only have a single junit-platform.properties in our runtime classpath since JUnit does not guarantee which one will be loaded (junit-team/junit5#2794).

@github-actions github-actions bot added core Kafka Broker tools small Small PRs labels Oct 7, 2024
@mumrah mumrah added the build Gradle build or GitHub Actions label Oct 7, 2024
Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

If I recall correctly, junit-platform.properties is used to generate the correct test naming, such as quorum=kraft and quorum=zk, for the older test infrastructure. In short, junit-platform.properties can be removed once ZK is completely removed from the tests.

@mumrah
Copy link
Member Author

mumrah commented Oct 8, 2024

Same four tests are failing on both Java version:

Found 4 test failures:
FAILED ❌ AppInfoParserTest > testUnregisterAppInfoUnregistersMetrics()
FAILED ❌ AppInfoParserTest > testRegisterAppInfoRegistersMetrics()
FAILED ❌ CommonNameLoggingTrustManagerFactoryWrapperTest > testCommonNameLoggingTrustManagerWithExpiredEndCert()
FAILED ❌ CommonNameLoggingTrustManagerFactoryWrapperTest > testCommonNameLoggingTrustManagerWithExpiredEndCertWithCA()

🤔

Edit: Looks like some clients tests make assumptions about the classpath. I'll revert the changes to :clients module.

@mumrah
Copy link
Member Author

mumrah commented Oct 8, 2024

If I recall correctly, junit-platform.properties is used to generate the correct test naming
Yes, I think it's used in general for parameterized tests, not just ZK/KRaft.

This file also lets us register JUnit plugins easily. This may be coming soon in a future PR :)

@chia7712
Copy link
Contributor

chia7712 commented Oct 8, 2024

This file also lets us register JUnit plugins easily. This may be coming soon in a future PR :)

I just open a jira to remove the file 😄

Let me close it

build.gradle Outdated Show resolved Hide resolved
@mumrah mumrah changed the title MINOR Consolidate to a single junit-platform.properties MINOR Consolidate junit-platform.properties Oct 8, 2024
@mumrah mumrah merged commit 51482fa into apache:trunk Oct 8, 2024
6 checks passed
@mumrah mumrah deleted the minor-one-junit-properties branch October 8, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Gradle build or GitHub Actions core Kafka Broker small Small PRs tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants