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: Refactor ShareConsumerTest to use ClusterTestExtensions. #18656

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

smjn
Copy link
Contributor

@smjn smjn commented Jan 21, 2025

  • Currently, the ShareConsumerTest suite uses KafkaClusterTestKit to create kafka clusters and assert various scenarios. However, the current implementation is not open for parameterization.
  • In this PR we have modified the impl to make use of ClusterTestExtensions and associated abstractions which makes the tests cleaner and easy to parameterise.
  • We have also added ShareConsumer instance creation factory method in ClusterInstance interface.

@github-actions github-actions bot added triage PRs from the community core Kafka Broker tests Test fixes (including flaky tests) labels Jan 21, 2025
@apoorvmittal10 apoorvmittal10 self-requested a review January 21, 2025 15:05
@AndrewJSchofield AndrewJSchofield added KIP-932 Queues for Kafka and removed triage PRs from the community labels Jan 21, 2025
@smjn smjn marked this pull request as ready for review January 21, 2025 15:22
this.cluster = cluster;
}

private void setup() {
Copy link
Member

Choose a reason for hiding this comment

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

It would be preferable if it was possible to put this in a @BeforeEach method. Is that no longer an option, even with code gymnastics?

Copy link
Contributor Author

@smjn smjn Jan 21, 2025

Choose a reason for hiding this comment

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

@AndrewJSchofield I think not - since the clusterInstance is not populated at that time. It shows up as null.
Using @BeforeEach - it throws an exception

org.opentest4j.AssertionFailedError
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:46)
	at org.junit.jupiter.api.Assertions.fail(Assertions.java:161)
	at kafka.test.api.ShareConsumerTest.setup(ShareConsumerTest.java:144)
	...
Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.kafka.common.test.KafkaClusterTestKit.waitForReadyBrokers()" because "this.clusterTestKit" is null
	at org.apache.kafka.common.test.api.RaftClusterInvocationContext$RaftClusterInstance.waitForReadyBrokers(RaftClusterInvocationContext.java:219)
	at kafka.test.api.ShareConsumerTest.setup(ShareConsumerTest.java:136)
	... 17 more

I checked a couple of implementations and they are using this approach to perform pre-test initializations. The internal KafkaClusterTestKit object is not populated when beforeEach is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mumrah Is there something I am missing?

Copy link
Member

Choose a reason for hiding this comment

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

At the moment, we can't use the ClusterInstance inside a @BeforeEach with @ClusterTest. I'll see if we can fix that in a safe way.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #18662 to add support for this. For now, we can do the explicit "setup" call as done in this PR.

@smjn smjn requested a review from AndrewJSchofield January 21, 2025 16:00
Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved core Kafka Broker KIP-932 Queues for Kafka tests Test fixes (including flaky tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants