KAFKA-12173 Migrate streams:streams-scala module to JUnit 5 by chia7712 · Pull Request #9858 · apache/kafka

chia7712 · 2021-01-11T10:59:17Z

issue: https://issues.apache.org/jira/browse/KAFKA-12173

replace org.junit.Assert by org.junit.jupiter.api.Assertions
replace org.junit by org.junit.jupiter.api
replace Before by BeforeEach
replace After by AfterEach
remove ExternalResource from all scala modules
add explicit AfterClass/BeforeClass to stop/start EmbeddedKafkaCluster

Noted that this PR does not migrate stream module to junit 5 so it does not introduce callback of junit 5 to deal with beforeAll/afterAll. The next PR of migrating stream module can replace explicit beforeAll/afterAll by junit 5 extension. Or we can keep the beforeAll/afterAll if it make code more readable.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2021-01-14T14:15:51Z

For the Stream modules, we should get one of the committers that is more involved in that part. @mjsax and @vvcephei are two possible candidates.

mjsax · 2021-01-27T03:48:10Z

This PR is huge and contains a lot of reformatting. Is this required? Very tedious to review. Why are there so many Java class changes -- this PR does not migrate Java tests to JUnit5? Could this PR be split into multiple smaller ones to make it easier to review?

vvcephei

Woah, thanks for all this work, @chia7712 !

I agree it's a little on the large side for a PR, but it looks like it was necessary, since changing EmbeddedKafkaCluster dragged in all the integration tests.

I'm assuming we had to ditch Scalatest stuff like Flatspec because the JUnitRunner isn't compatible with JUnit5? It's fine with me either way. We were writing those Scala tests in exactly the same way we write the Java ones, might as well use the same testing framework, too.

All in all, these changes look good to me. Please feel free to merge whenever you're satisfied with the test results, and thanks again, @chia7712 !

chia7712 · 2021-03-23T03:33:37Z

@mjsax Sorry for lazy response. I did not notice your feedback until @vvcephei touch this PR :(

This PR is huge and contains a lot of reformatting. Is this required? Very tedious to review. Why are there so many Java class changes -- this PR does not migrate Java tests to JUnit5? Could this PR be split into multiple smaller ones to make it easier to review?

We are migrate code to Junit 5 by module in order to avoid huge patch (although this patch is still big ...). The root cause of making this tedious patch is that stream-scale depends on stream module (EmbeddedKafkaCluster, to be exact). Unfortunately, the Junit APIs used by EmbeddedKafkaCluster belong to Junit 4 and those APIs are not compatible to Junit 5. That is why this patch has to reformat a lot of java code of streams module.

chia7712 · 2021-03-23T03:34:01Z

@vvcephei Thanks for your feedback. I will fix conflicting files.

chia7712 · 2021-03-24T08:01:31Z

@ijuma @mjsax I'd like to merge this PR and then I can migrate EmbeddedKafkaCluster(streams module) to junit 5 in order to remove duplicate code. It would be helpful if you could give another look. thanks!

ijuma · 2021-03-24T12:01:42Z

@chia7712 If @vvcephei reviewed it, I think you can go ahead and merge.

) 1. replace org.junit.Assert by org.junit.jupiter.api.Assertions 2. replace org.junit by org.junit.jupiter.api 3. replace Before by BeforeEach 4. replace After by AfterEach 5. remove ExternalResource from all scala modules 6. add explicit AfterClass/BeforeClass to stop/start EmbeddedKafkaCluster Noted that this PR does not migrate stream module to junit 5 so it does not introduce callback of junit 5 to deal with beforeAll/afterAll. The next PR of migrating stream module can replace explicit beforeAll/afterAll by junit 5 extension. Or we can keep the beforeAll/afterAll if it make code more readable. Reviewers: John Roesler <vvcephei@apache.org>

related PR: apache#9858

#10655) Related PR where the `scalatest` usage was removed: #9858 Reviewers: Ismael Juma <ismael@juma.me.uk>

ableegoldman · 2021-08-25T01:42:58Z

+
+    @AfterClass
+    public static void closeCluster() {
+        CLUSTER.stop();


Hey @chia7712 , sorry for never bothering to take a look at this PR until just now, but I had a question about this. I know it was necessary to remove the ExternalResource feature that used to be responsible for calling start() and stop() for us in the integration tests since @ClassRule was removed in JUnit5, but that was really quite a loss since this now leaves us vulnerable to

resource leaks due to forgetting to clean up the EmbeddedKafkaCluster in an integration test, or doing so in an incorrect way (eg such that a test failure might skip the cleanup stage, a mistake that we've certainly encountered in our tests in the past)

breaking compatibility of integration tests across older branches, so that if we ever need to backport a fix that includes an integration test -- which many will/should do -- we can easily break the build of older branches by accident. See for example #11257: aka the reason I started digging into this 🙂 . Even if we remember to fix this during the backport, it's just an extra hassle.

Now I'm certainly not an expert in all things JUnit, but a cursory glance suggests we can replicate the old behavior in which the EmbeddedKafkaCluster is automatically started/stopped without the need for this @Before/AfterClass boilerplate code in every integration test. I believe it's possible to do so using the @Extension/ExtendWith annotations. Can we try to port the EmbeddedKafkaCluster back to an automated lifecycle with these so we can clean up the Streams integration tests?

cc @ijuma @vvcephei who may be more familiar with these constructs and how/when/why to use them

cc also @showuon -- would you be interested in taking a shot at this?

I'll check it! :)

https://issues.apache.org/jira/browse/KAFKA-13230 is created for this issue. Need more discussion to make sure what we really want.

chia7712 requested a review from ijuma January 11, 2021 10:59

KAFKA-12173 Migrate streams:streams-scala module to JUnit 5

d2b3cf2

chia7712 force-pushed the KAFKA-12173 branch from f44ac89 to d2b3cf2 Compare January 14, 2021 02:50

chia7712 mentioned this pull request Jan 14, 2021

KAFKA-7341 Migrate core module to JUnit 5 #9855

Merged

3 tasks

chia7712 added 5 commits January 14, 2021 22:24

Merge branch 'trunk' into KAFKA-12173

bda5610

Merge branch 'trunk' into KAFKA-12173

77d2b1e

remove scalatest

f40695e

Merge branch 'trunk' into KAFKA-12173

1bf002b

Merge branch 'trunk' into KAFKA-12173

24317ad

mjsax added streams tests Test fixes (including flaky tests) labels Jan 27, 2021

vvcephei approved these changes Mar 23, 2021

View reviewed changes

chia7712 added 2 commits March 23, 2021 11:34

Merge branch 'trunk' into KAFKA-12173

83762f5

fix failed tests

bda798f

chia7712 merged commit 9af8195 into apache:trunk Mar 24, 2021

dejan2609 mentioned this pull request May 2, 2021

MINOR: dozen of version upgrades (for both dependencies and gradle plugins) #10627

Closed

dejan2609 added a commit to dejan2609/kafka that referenced this pull request May 8, 2021

MINOR: redundant dependency (scalatest) is removed out of libraries list

b07f266

related PR: apache#9858

dejan2609 mentioned this pull request May 8, 2021

MINOR: redundant dependency (scalatest) is removed out of libraries list #10655

Merged

ijuma pushed a commit that referenced this pull request May 10, 2021

MINOR: Remove unused scalatest definition from dependencies.gradle (

9bec362

#10655) Related PR where the `scalatest` usage was removed: #9858 Reviewers: Ismael Juma <ismael@juma.me.uk>

ableegoldman reviewed Aug 25, 2021

View reviewed changes

chia7712 deleted the KAFKA-12173 branch March 25, 2024 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12173 Migrate streams:streams-scala module to JUnit 5#9858

KAFKA-12173 Migrate streams:streams-scala module to JUnit 5#9858
chia7712 merged 8 commits intoapache:trunkfrom
chia7712:KAFKA-12173

chia7712 commented Jan 11, 2021 •

edited

Loading

Uh oh!

ijuma commented Jan 14, 2021

Uh oh!

mjsax commented Jan 27, 2021

Uh oh!

vvcephei left a comment

Uh oh!

chia7712 commented Mar 23, 2021

Uh oh!

chia7712 commented Mar 23, 2021

Uh oh!

chia7712 commented Mar 24, 2021

Uh oh!

ijuma commented Mar 24, 2021

Uh oh!

ableegoldman Aug 25, 2021

Uh oh!

ableegoldman Aug 25, 2021

Uh oh!

showuon Aug 26, 2021

Uh oh!

showuon Aug 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

chia7712 commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Jan 14, 2021

Uh oh!

mjsax commented Jan 27, 2021

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Mar 23, 2021

Uh oh!

chia7712 commented Mar 23, 2021

Uh oh!

chia7712 commented Mar 24, 2021

Uh oh!

ijuma commented Mar 24, 2021

Uh oh!

ableegoldman Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

ableegoldman Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

showuon Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

showuon Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chia7712 commented Jan 11, 2021 •

edited

Loading