KAFKA-7341 Migrate core module to JUnit 5#9855
Conversation
|
FYI, I started reviewing this so please no force pushes. :) |
|
Quick update: I reviewed over 200 files, so I think I'll be able to complete this later today. |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Went through a few more lines and left more comments. 21 to go. :)
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
A few more comments. About 10 files to go.
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Finally completed the review, hopefully last set of comments below. Thanks for your effort and persistence!
| @BeforeEach | ||
| override def setUp(): Unit = { | ||
| // junit 5 can't make parametered BeforeEach so we override setup to do nothing as we do setup cluster in test case | ||
| } |
There was a problem hiding this comment.
This is not ideal. Since we have a single test, maybe we can do the parameterization manually within the test.
There was a problem hiding this comment.
That requires to modify server configs "before" the @BeforeEach. It seem to me the only way to address it is to separate the test class. WDYT?
There was a problem hiding this comment.
I was thinking we could use dynamic tests instead: https://www.baeldung.com/junit5-dynamic-tests#improving-the-dynamictest-using-java-8-features
There was a problem hiding this comment.
The last commit rewrite the test by nested test and it seems to me that is more readable than dynamic test. WDYT?
There was a problem hiding this comment.
Not sure. I think I would prefer 4 hardcoded test methods calling a private method with two booleans. Seems like it does the job in a simpler way.
There was a problem hiding this comment.
Basically:
testAutoCreationEnabledtestBrokerAutoCreationEnabledtestClientAutoCreationEnabledtestAutoCreationDisabled
…to checkInvalidArgs
| @Test | ||
| def testAutoCreationDisabled(): Unit = testAutoTopicCreation(false, false) | ||
|
|
||
| private def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = { |
There was a problem hiding this comment.
With this, you don't need the separate TestCase, right? You can rely on standard JUnit lifecycle methods.
There was a problem hiding this comment.
Oh, this doesn't solve the problem. In that case, my suggestion was bad and what you had before was better. Sorry about that.
There was a problem hiding this comment.
the related code is reverted.
|
Will you go ahead and merge then? |
give me one second. the QA is always fails on JDK 8. I'm testing it on my local. |
| List[Integer](brokerId, brokerId + 1, brokerId + 2), | ||
| List[Integer](brokerId, brokerId + 1, brokerId + 2), | ||
| List.empty[Integer], List.empty[Integer], Seq.empty[Int], false), | ||
| List.empty[Integer], List.empty[Integer], Seq.empty[Int], Boolean.box(false)), |
There was a problem hiding this comment.
For the future (no need to change right now), I think it's a bit cleaner to do false: java.lang.Boolean. The compiler figures out the most appropriate way to convert.
2 participants
issue: https://issues.apache.org/jira/browse/KAFKA-7341
This PR includes following changes.
org.junit.Assertbyorg.junit.jupiter.api.Assertionsorg.junitbyorg.junit.jupiter.apiScalaTestfrom core dependenciesorg.junit.runners.Parameterizedbyorg.junit.jupiter.params.ParameterizedTestorg.junit.runners.Parameterized.Parametersbyorg.junit.jupiter.params.provider.{Arguments, MethodSource}BeforebyBeforeEachAfterbyAfterEachCommitter Checklist (excluded from commit message)