confluentinc / kafka-rest

Confluent REST Proxy for Kafka
https://docs.confluent.io/current/kafka-rest/docs/index.html
Other
38 stars 643 forks source link

KREST-9993 Migrate integration tests to kraft #1228

Closed trnguyencflt closed 10 months ago

trnguyencflt commented 10 months ago

What

This PR migrates:

Recommendation for the reviewers

As the change in this PR are quite big (59 files, +/-1800), it might look intimidating for reviewing. However, it should be easy to review if you follow those steps:

  1. Reviewing ClusterTestHarness
  2. Reviewing QuorumControllerFixture and other files under testing package
  3. Reviewing pom.xml changes
  4. Then for the integration tests, those files are repetitive with the change of
    1. setUp() -> setUp(TestInfo)
    2. tests now become parameterized tests with ValueSources to be either zk or kraft, note that the param String quorum in test method is required.

References

https://confluentinc.atlassian.net/browse/KREST-9993

msn-tldr commented 10 months ago

@trnguyencflt Is there a way to make this change non-breaking? To

  1. Avoid operational toil for us and our downstream deps, by not breaking builds.
  2. Get more confidence by getting green signal in this PR.

Leaving two methods that annotate with @BeforeEach (if we would keep setUp() and add setUp(TestInfo)) could be problematic as Junit5 will pickup any function (including super class) with this annotation to run before a test (with no particular order), thus we could introduce unexpected behaviour in tests of downstream projects (suppose that the test only override setUp()).

How about having both setup() & setup(Testinfo)? Apply @BeforeEach to only setup(Testinfo). And then setup() could call setup(Testinfo) with a custom TestInfo object that setups the calling test for ZK.

trnguyencflt commented 10 months ago

@msn-tldr

Apply @BeforeEach to only setup(Testinfo). And then setup() could call setup(Testinfo) with a custom TestInfo object that setups the calling test for ZK.

I tried this idea, unfortunately it doesn't work 😞 because JUnit5 will look for every method with @BeforeEach annotation, including superclasses, so if any subclass override setup() and with @BeforeEach annotation, the tests in that subclass end up calling two setUp, which create two controller instances, and will lead to out of memory issue because one of them will not be teardown.

trnguyencflt commented 10 months ago

@msn-tldr

Apply https://github.com/beforeeach to only setup(Testinfo). And then setup() could call setup(Testinfo) with a custom TestInfo object that setups the calling test for ZK.

Actually, I think this might work in the sense that I'll update all the integration tests of downstream projects anyway, so no breaking change and we are sure that there isn't memory leak problem introduced.

trnguyencflt commented 10 months ago

Actually, I think this might work in the sense that I'll update all the integration tests of downstream projects anyway, so no breaking change and we are sure that there isn't memory leak problem introduced.

@msn-tldr unfortunately, this doesn't work because this goes against junit5 @BeforeEach recommendation of not having more than two methods that depends on each other.