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

Remove unused @ClassRule container in KafkaContainerTest and use cp-kafka:6.2.1 as default #4564

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

kiview
Copy link
Member

@kiview kiview commented Oct 8, 2021

Since we still saw some flakiness from KafkaContainerTest in our CI, I tried to remove some of the elements that I thought of as unnecessary.

The @ClassRule annotated container was never used in any tests and was just left there for the sake of including its code in the documentation. IMO it is not useful to add test framework integration to each container module documentation, since it is not specific for the container.

The update to cp-kafka:6.2.1 was done to use a container-aware JVM so we have better guarantees for the memory configuration. Accordingly, I changed the test testConfluentPlatformVersion6 test to check instead compatibility with platform version 5, although we could consider removing this as well?

I also removed the test for the deprecated tag version constructor, but if you still want to guard against potential regressions, we can of course keep it.

* Also, upgrade to cp-kafka:6.2.1 as default image in tests to use a container-aware JVM.
* Instead of explicitly checking for platform 6 compatibility, the test now checks for platform 5 compatibility instead.
* Remove a test for the deprecated constructor.
@@ -50,7 +45,7 @@ public void testUsage() throws Exception {
public void testUsageWithSpecificImage() throws Exception {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think no real need to keep the general testUsage() as well as the testUsageWithSpecificImage test. Since testUsageWithSpecificImage is used for inlining code in the docs, I would keep this one.

@rnorth
Copy link
Member

rnorth commented Oct 14, 2021

The @ClassRule annotated container was never used in any tests and was just left there for the sake of including its code in the documentation. IMO it is not useful to add test framework integration to each container module documentation, since it is not specific for the container.

👍

The update to cp-kafka:6.2.1 was done to use a container-aware JVM so we have better guarantees for the memory configuration. Accordingly, I changed the test testConfluentPlatformVersion6 test to check instead compatibility with platform version 5, although we could consider removing this as well?

Seems sensible to me

I also removed the test for the deprecated tag version constructor, but if you still want to guard against potential regressions, we can of course keep it.

I think it would be a good idea to keep this, TBH...

@kiview
Copy link
Member Author

kiview commented Oct 14, 2021

Sure, I brought the test for the legacy constructor back.

@kiview kiview added this to the next milestone Oct 14, 2021
@kiview kiview merged commit 2174928 into master Oct 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the kafka-test-trimdown branch October 14, 2021 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants