KAFKA-14001: Migrate streams module to JUnit 5 - Part 1

[clolov]

This pull request addresses https://issues.apache.org/jira/browse/KAFKA-14001. It is the first of a series of pull requests which address the move of Kafka Streams tests from JUnit 4 to JUnit 5.

The below description is kept to track the history of this pull request.

This is the first part of KAFKA-7342 (https://issues.apache.org/jira/browse/KAFKA-7342). Here are the decisions I made in this change:
* Do not (yet) migrate files using Parameterized because they require more in-depth changes. A list of the files which I haven't changed can be found here (1).
* Prefix assertTrue, assertFalse, assertNull etc. with Assertions.
* Run IntelliJ's Optimize Imports and streams:spotlessApply on the streams module.

1. List of untouched tests. Once these are migrated then JUnit4 should no longer be used in Kafka Streams

streams/src/test/java/org/apache/kafka/streams/integration/RocksDBMetricsIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/EOSUncleanShutdownIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KTableKTableForeignKeyJoinMaterializationIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/EosIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KStreamRepartitionIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/PositionRestartIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/SlidingWindowedKStreamIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StreamTableJoinTopologyOptimizationIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamSlidingWindowAggregateTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/ActiveTaskCreatorTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingWindowBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/MergedSortedCacheWrappedWindowStoreIteratorTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimestampedSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/SessionStoreFetchTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedKeyValueStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreFetchTest.java
streams/src/test/java/org/apache/kafka/streams/integration/GlobalKTableEOSIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KTableKTableForeignKeyJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/ResetPartitionTimeIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StreamStreamJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/TableTableJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/TimeWindowedKStreamImplTest.java
streams/src/test/java/org/apache/kafka/streams/integration/EosV2UpgradeIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KTableEfficientRangeQueryTest.java
streams/src/test/java/org/apache/kafka/streams/integration/RangeQueryIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StandbyTaskEOSIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/SuppressionDurabilityIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamWindowAggregateTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorStateManagerTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreToProcessorContextAdapterTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/TimestampedKeyValueStoreMaterializerTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueIteratorFacadeTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/MergedSortedCacheWrappedWindowStoreKeyValueIteratorTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBWindowStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedWindowStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/RepartitionTopicsTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingSessionBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ReadOnlyKeyValueStoreFacadeTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBSessionStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/SessionKeySchemaTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedKeyValueBufferTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/WindowKeySchemaTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StreamTableJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/TimeWindowedKStreamIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/graph/TableSourceNodeTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignorTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimestampedWindowBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ListValueStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ReadOnlyWindowStoreFacadeTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/SessionStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedWindowStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/integration/AbstractJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractSessionBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractDualSchemaRocksDBSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractWindowBytesStoreTest.java

Committer Checklist (excluded from commit message)

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

[clolov]

Polite ask for a review from @divijvaidya @dengziming @Kvicii @mimaison. The decisions I have outlined in the description are reversible should there be objections to them :)

[Kvicii]

@clolov thanks your PR, I think we should multiple modify.
@divijvaidya @dengziming @mimaison Do you have any better suggestion?

[divijvaidya]

Thanks you submitting this change @clolov. This would greatly help us in moving towards getting rid of JUnit4 completely.

1\ To make the code review easier, could we please tackle the Run IntelliJ's Optimize Imports and streams:spotlessApply on the streams module. in a separate review? I would drastically decrease the number of files changed and help us reviewing the code in a better manner.

2\ I agree with (and appreciate) your incremental approach towards this migration. We can handle the tests using parameterised separately.

3\ It would be helpful if you can document the high level changes required for the migration from Junit4 to Junit5 in the description of this PR.

[clolov]

Okay, I will work on 1 and 3, thank you for the review.

[ahmed82]

I know a lot of formatting involved, otherwise good job.

[mimaison]

Thanks for the PR!
Since this PR only touches Streams, I'll let committers focused on Streams review it.

[OneCricketeer]

Worth putting in a separate PR, but have you tried enabling the Jupiter parallel test runner? When I ran it on a work project, it improved build times by an order of magnitude

[cadonna]

@clolov Thank you for the PR!
I agree with @divijvaidya about doing the reformatting in a separate PR. Could you also try to subdivide the PR into smaller PRs? Reviewing a 6500 line PR is never fun. Sizes around 500 are acceptable.

[ijuma]

Worth putting in a separate PR, but have you tried enabling the Jupiter parallel test runner? When I ran it on a work project, it improved build times by an order of magnitude

We use multiple forks at the gradle level, so we should not enable this.

A couple more comments:

1. Let's revert the formatting changes.
2. Do not prefix with Assertions.
3. Mention in the PR title that these changes are for the streams module(s).

[clolov]

Hello! Thank you to everyone who has left a comment and suggestions for improvement. In the next few days I will aim to rework this pull request. In summary:

• I will revert the import reordering
• I will not prefix assertions with Assertions
• I will mention that these changes are for the streams module
• I will split the PR into multiple ones so to stick to the <= 500 lines rule

[clolov]

@cadonna @ijuma @divijvaidya @Kvicii I hope the latest commits address all of the suggestions above :)

[divijvaidya]

LGTM! @cadonna this is ready for your review now.

[clolov]

Politely bumping the review @cadonna

[cadonna]

Thanks for the PR, @clolov !

LGTM!

I have one question.

[cadonna]

Why did you not put this code into the setup() method?

[clolov]

I cannot remember, so maybe there was no reason. When I was rebasing I noticed that this test has since been deleted (or moved to org.apache.kafka.connect.integration) so I believe it is safe to say I do not need to address this?

[cadonna]

@clolov could you please rebase your PR since there are conflicts?

[clolov]

Hey @cadonna, I rebased on top of trunk. Do let me know if there is something else you would like me to address :)

[cadonna]

LGTM!

Thanks a lot for the PR and your patience, @clolov !

[cadonna]

Once I get acceptable builds, I am going to merge this PR!

[cadonna]

Failures are not related

[cadonna]

@clolov @divijvaidya I just checked and these tests are not run in the builds. I have also another PR in streams with tests that use junit5 and they are also not run in the builds. Does anybody of you know what is going on here?
I do not want to to go on with the migration from junit4 to junit5 if tests are not run until we have migrated all tests.

Here the tests that are run: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12285/5/testReport/org.apache.kafka.streams.integration/

[clolov]

I don't know, but I can have a look in the upcoming days.

[divijvaidya]

Hey @clolov @cadonna
Yes, I have figured out why the newly converted tests to JUnit5 are not running. There are two reasons to it:

1. There is an overall block to use JUnit5 for streams project at https://github.com/apache/kafka/blob/trunk/build.gradle#L324. [Action] We need to remove streams from here.
2. The dependency chain does not pull in Jupiter engine dependency instead it pulls in Jupiter API dependency at https://github.com/apache/kafka/blob/trunk/build.gradle#L1839 [Action] We need to replace this with testImplementation libs.junitJupiter

I made the above two changes and ensured that the modified test is run using ./gradlew streams:test --tests AdjustStreamThreadCountTest

With the above changes, we will still have tests which won't execute during build because at https://github.com/apache/kafka/blob/trunk/build.gradle#L466, we will only run tests with "integration" tag. A simple way to fix it is to run all Junit4 and Junit5 tests using the Jupiter platform. It is allowed if we use the JUnitVintageEngine. I will file a pull request soon to fix what I mentioned above.

[ijuma]

For other modules, we did the JUnit 5 migration in one PR to avoid this problem. As @divijvaidya mentioned, the build expects each module to be JUnit 4 or JUnit 5.

[clolov]

So then would you like me to coalesce #12301 and #12302 into one, add all remaining tests, make the changes to build.gradle and all other files into a single pull request?

[divijvaidya]

@clolov we don't have to do it in one go. We can create a new project/task in build.gradle which runs streams integration tests tagged with @tag('integration') using Junit5. When we are done with Junit4 to Junit5 migration, we can remove that project/task use the usual integrationTest task.

[cadonna]

@clolov Let's first try to make it work with the junit vintage engine. I am not clear if there is still an issue with powermock and junit5 and it seems like Streams still uses powermock.

[cadonna]

Personally, I would prefer to make the migration incrementally.

[clolov]

Hello @cadonna! I spoke with Divij offline and we found a way to run both JUnit 4 and JUnit 5 tests while the migration is going on. The pull request which should enable this is #12441.

[ijuma]

Doing things incrementally over a long period of time is a bit confusing since you cannot enforce that JUnit 4 or JUnit 5 is used at any point in time. My take is that it's simplest if the conversion is done quickly. However, it's best to convert the tests to Mockito first - that part is more complicated and can be done incrementally.

[cadonna]

I agree that the migration should be done quickly.
As far as I understand @clolov plans to do the whole migration quickly. Is this correct? If this is the case, I do not see the issue with making the migration incrementally for the sake of smaller PRs that are easier reviewed.
If you create one big PR for the migration you might run into the issue that during the review other PRs add tests in JUnit4. You would need to rebase and extend the PR to get green builds.
I agree that we should get the migration from EasyMock/PowerMock to Mockito done before we do the migration from JUnit4 to JUnit5 to make the reviews simpler.
So our option now are:

1. Revert this PR, do the migration to Mockito incrementally, and do the migration to JUnit 5 in one shot
2. Do the change in the builds to allow JUnit 5 along with JUnit 4 and do both migrations incrementally.
3. Do the change in the builds to allow JUnit 5 along with JUnit 4, migrate to Mockito, and then migrate all tests to JUnit 5.

For options 2 and 3 committers should be made aware to only accept JUnit 5 tests in new PRs. That should be easy for Streams.

I am in favor of 3 because it happened to me that I already added some tests in JUnit5 in some of my PRs because I was not aware that JUnit 4 and JUnit 5 do not run alongside each other in our builds. Migrating the JUnit 5 tests to JUnit 4 in my PRs do not seem to be a big deal, though.

I am also fine with option 1. Option 2 seems the oddest to me.

I would like to know the preferences of @clolov and @divijvaidya since they are driving this migration?

[ijuma]

I am fine with 3 as long as we don't make changes that make it worse for modules that have already converted to JUnit 5.

[clolov]

Divij and I also prefer option 3 because of the same reasons posted by Bruno. So the next steps on our side are:

1. Address comments on KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run #12441 which should bring us back to a state where tests are ran on every pull request
2. Continue with the smaller pull requests for unit test migration from JUnit 4 to JUnit 5 which do not depend on PowerMock/EasyMock
3. Raise (multiple) pull request(s) to move PowerMock/EasyMock tests in Streams to Mockito
4. Once 3 is done prepare pull request(s) for them to be upgraded to JUnit 5
5. Cleanup the setup which allows JUnit 4 tests to run in Streams + any new tests which might have appeared in the interim

I will be away until 2nd of August so Divij will provide updates on #12441. Once I am back I will continue with the plan as detailed above. Of course, if there are any suggestions for it to be done otherwise I am more than happy to discuss those.

[ijuma]

Option 3 says not to continue JUnit 5 migrations though (step 2) - it says to do the mockito migration first.

[clolov]

Well somewhere between reading the comments, having a discussion and typing my response I confused the options.

I don't see why allowing JUnit 4 tests to run alongside JUnit 5 tests albeit with a separate engine would be problematic while moving between them. After all, that's exactly what the vintage engine is supposed to do and what I as a developer want as an experience. I furthermore don't think enabling it on the overarching project level would be problematic. After all, there are two engines and JUnit 5 is smart enough to know which tests to run with which engine. To confirm/deny this statement I will add tests reports as Divij has suggested to the pull request for enabling the above change once I am back. What I am expecting the report to show is that more tests have been ran for streams and there has not been a change in the tests for other modules (barring flaky tests).

As to "PowerMock/EasyMock -> Mockito done first", "pure JUnit 4 -> JUnit 5 done first" I don't see why order matters. The sets of pure JUnit 4 tests and JUnit 4 + PowerMock/EasyMock tests are disjoint and can be addressed separately. In other words, I do not see reasons to reject pull requests (such as this one) given they advance the state of the world to JUnit 5.

As a summary, let me focus my efforts on #12441 if it has not been agreed upon and merged by the time I am back. I believe we all agree that is a prerequisite to any incremental change. After that I will be opening pull requests for moving from PowerMock/EasyMock to Mockito and whatever tests can already be moved to JUnit 5 and you can review and merge them in whatever order you prefer :)