KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run by clolov · Pull Request #12441 · apache/kafka

clolov · 2022-07-26T12:38:46Z

This pull request addresses the problem reported in #12285 and tracked in https://issues.apache.org/jira/browse/KAFKA-14108

divijvaidya · 2022-07-26T12:49:07Z

@clolov

don't we need to remove "streams" from

  def shouldUseJUnit5 = !(["runtime", "streams"].contains(it.project.name))

as well?

please add the test report for "before" the change and for "after" the change. This would help us validate that number of tests run "after" this change is greater than or equal to "before" the change.

cadonna · 2022-07-26T13:31:19Z

Thanks for the PR, @clolov !

The builds have errors:

* What went wrong:
[2022-07-26T12:41:05.041Z] A problem occurred evaluating root project 'Kafka_kafka-pr_PR-12441'.
[2022-07-26T12:41:05.041Z] > Cannot convert a null value to an object of type Dependency.
[2022-07-26T12:41:05.041Z]   The following types/formats are supported:
[2022-07-26T12:41:05.041Z]     - Instances of Dependency.
[2022-07-26T12:41:05.041Z]     - String or CharSequence values, for example 'org.gradle:gradle-core:1.0'.
[2022-07-26T12:41:05.041Z]     - Maps, for example [group: 'org.gradle', name: 'gradle-core', version: '1.0'].
[2022-07-26T12:41:05.041Z]     - FileCollections, for example files('some.jar', 'someOther.jar').
[2022-07-26T12:41:05.041Z]     - Projects, for example project(':some:project:path').
[2022-07-26T12:41:05.041Z]     - ClassPathNotation, for example gradleApi().

ijuma · 2022-07-26T13:54:32Z

build.gradle

+        // KAFKA-14109
+        // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.
+        // junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete.
+        includeEngines "junit-vintage", "junit-jupiter"


We don't want to do this for all modules since most of them have already been converted.

Is there a disadvantage/side-effect if we do it for all the modules?

From what I understand (please correct me if I am wrong), loading the vintage engine for JUnit5 tests won't have any side effects since they would be run with jupiter engine anyways. Vintage engine only runs Junit4 tests in a Junit5 platform. It does not impact already converted Junit5 tests running on Junit5 platform.

I am advocating for this because it keeps the changes minimal and simplified here. Given that it is a temporary transient stage (we already have PRs out for most of the test conversion to JUnit5), I would preferr minimal changes so that reverting them is easier.

cadonna · 2022-07-26T13:54:34Z

@clolov I also found a mistake in #12285:
In KTableSourceTopicRestartIntegrationTest line 103 should be

sourceTopic = SOURCE_TOPIC + "-" + testInfo.getTestMethod().map(Method::getName).orElse("");`

instead of

sourceTopic = SOURCE_TOPIC + "-" + testInfo.getTestMethod().map(Method::getName);

otherwise the test does not run wince the topic name contains illegal characters.

ijuma · 2022-07-26T13:54:57Z

build.gradle

    if (shouldUseJUnit5) {
      useJUnitPlatform {
        includeTags "integration"
+        // KAFKA-14109


We don't typically include jira references in the code like this (it pollutes the code and goes stale typically).

I second Ismael's comment.

Addressed the comment in the latest revision.

ijuma · 2022-07-26T13:56:33Z

gradle/dependencies.gradle

+  // KAFKA-14109
+  // The below dependency is needed for compiling JUnit 4 tests.
+  // It can be safely removed once all of streams has moved to JUnit 5.
+  junit4: "junit:junit:$versions.junit4",


This explanation is odd - compilation works currently and we don't have this dependency.

I did some tests and discovered that junitVintageEngine pulls in JUnit 4. That means if we move junitVintageEngine back to the testImplementation dependencies compilation of JUnit4 tests works.

That makes sense. There should be no need for depending on the old junit4 jars directly.

Removed the dependency in latest revision.

cadonna

@clolov Thanks for the PR!

I had some suggestions how you can ensure that all tests are run without changing already migrated modules.

cadonna · 2022-07-28T15:59:25Z

build.gradle

    if (shouldUseJUnit5) {
      useJUnitPlatform {
        includeTags "integration"
+        // KAFKA-14109


I second Ismael's comment.

cadonna · 2022-07-28T16:06:36Z

build.gradle

      useJUnitPlatform {
        includeTags "integration"
+        // KAFKA-14109
+        // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.
+        // junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete.
+        includeEngines "junit-vintage", "junit-jupiter"
      }


I experimented a bit and found the following solution.

Suggested change

useJUnitPlatform {

includeTags "integration"

// KAFKA-14109

// Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.

// junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete.

includeEngines "junit-vintage", "junit-jupiter"

}

if (project.name.equals('streams')) {

useJUnitPlatform {

includeTags "integration"

includeTags 'org.apache.kafka.test.IntegrationTest'

includeEngines "junit-vintage", "junit-jupiter"

}

} else {

useJUnitPlatform {

includeTags "integration"

}

}

In this way, we can limit the change to Streams.
The caveat is that we need to replace @Category({IntegrationTest.class}) with @Tag{"integration"} in the integration tests that have already been migrated to JUnit 5.

Made the changes as suggested. Please see latest revision.

cadonna · 2022-07-28T16:08:40Z

build.gradle

      useJUnitPlatform {
        excludeTags "integration"
+        // KAFKA-14109
+        // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.
+        // junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete.
+        includeEngines "junit-vintage", "junit-jupiter"
      }


Here we can do similar as above but excluding the tags.

Suggested change

useJUnitPlatform {

excludeTags "integration"

// KAFKA-14109

// Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.

// junit-vintage (JUnit 4) can be removed once the JUnit 4 migration is complete.

includeEngines "junit-vintage", "junit-jupiter"

}

if (project.name.equals('streams')) {

useJUnitPlatform {

excludeTags "integration"

excludeTags 'org.apache.kafka.test.IntegrationTest'

includeEngines "junit-vintage", "junit-jupiter"

}

} else {

useJUnitPlatform {

excludeTags "integration"

}

}

cadonna · 2022-07-28T16:09:44Z

build.gradle

+    // KAFKA-14109
+    // The below compileOnly dependency is needed for JUnit 4 tests.
+    // It can be safely removed once all of streams has moved to JUnit 5.
+    testCompileOnly libs.junit4
+


As we said below this is not needed if you move libs.junitVintageEngine back to testImplementation.

Removed this dependency.

cadonna · 2022-07-28T16:11:35Z

build.gradle

-    testImplementation libs.junitJupiterApi
-    testImplementation libs.junitVintageEngine
+    testImplementation libs.junitJupiter
+    testImplementation libs.junitJupiterParams // needed for parameterized tests


I would prefer to add this when we need it during the migration of the parametrized tests. But I am also fine if it stays.

The inline comment is not needed.

I have removed this right now to keep this PR simple. Will add when needed.

divijvaidya · 2022-07-28T16:16:20Z

Thanks for the comments @cadonna @ijuma. Christo is on vacation right now (we are team mates) but I will take up this PR and file a revision addressing your comments tomorrow.

cadonna · 2022-07-28T16:19:51Z

@divijvaidya That would be great, because I start feeling uncomfortable having some integration tests not running on the builds.
Please also make sure to compare test reports before and after the change as you proposed.
Thanks a lot for taking over this work for Christo when he is on vacation!

cadonna · 2022-07-28T16:22:15Z

@divijvaidya Regarding your question

don't we need to remove "streams" from def shouldUseJUnit5 = !(["runtime", "streams"].contains(it.project.name)) as well?

Yes, we need.

divijvaidya · 2022-07-29T10:00:16Z

Changes

Replace with @Category({IntegrationTest.class}) with @Tag("integration")
Unblock streams project from using JUnitPlatform.
Use junit-vintage and junit-jupiter engine for executing tests of streams project. Tests formatted as Junit4 will automatically use junit-vintage and tests formatted as JUnit5 will automatically use junit-jupiter.
Minor change required in KTableSourceTopicRestartIntegrationTest.java after using junit-jupiter

Results

Result of executing ./gradlew :streams:test before & after this change.

Before

After

Note

After this change we are running more number of tests. This is because tests which were migrated to JUnit 5 were not being executed earlier.
**./gradlew :streams:integrationTest runs lesser number of tests than earlier. It only runs tests which have been converted to JUnit5. The other integration tests are run as UnitTests instead. I am trying to figure out why this is happening. **

Comments

Addressed concerns from @ijuma:

regarding not affecting other projects with this change (note that junit-vintage is being used for streams only)
regarding removal of addition of unnecessary dependencies (removed the unnecessary dependency)

Addressed concerns from @cadonna

regarding adding junitJupiterParams dependency only when needed (removed the dependency)

Next steps (separate PRs, in order)

Migrate tests using PowerMock to use Mockito
Migrate all tests to Junit5
Remove the Junit4 dependency completely from build.gradle

cadonna · 2022-07-29T10:20:38Z

@divijvaidya Thank you for the updates!

I am afraid you did a mistake when generating the test reports to test the changes. You changed task integrationTest and task unitTest but you generated the reports with streams:test. In task test no filtering is done on the tags integration and org.apache.kafka.test.IntegrationTest. Also the builds use integrationTest and unitTest to run the tests and not test. You should generate the reports with streams:integrationTest and streams:unitTest.

In my tests yesterday, I experienced that you cannot use @Tags("integration") (and filter on it) on integration tests that are written JUnit 4, but you need to leave @Category({IntegrationTest.class}) in the tests and use includeTags "org.apache.kafka.test.IntegrationTest" in the build file. For integration tests written in JUnit 5, you need to use @Tags("integration") in the tests and includeTags "integration" in the build file. See my comment #12441 (comment).

This reverts commit e5faaf9.

divijvaidya · 2022-07-29T11:59:49Z

.../test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java

    @BeforeEach
    public void before(final TestInfo testInfo) throws Exception {
-        sourceTopic = SOURCE_TOPIC + "-" + testInfo.getTestMethod().map(Method::getName);
+        sourceTopic = SOURCE_TOPIC + "-" + IntegrationTestUtils.safeUniqueTestName(getClass(), testInfo);


Note that the test fails without this change.

Yeah, thank you! That is known. See #12441 (comment)

divijvaidya · 2022-07-29T12:00:29Z

Updated the code. @cadonna should be ready for your review once the test run is complete.

Result for `./gradlew :streams:integrationTest`

Before

After

Result for `./gradlew :streams:unitTest`

Before

After

cadonna · 2022-07-29T12:11:21Z

@divijvaidya Thanks a lot for the update! This looks good!

However, there are some checkstyle issues due to some unused imports. Could you fix those?

cadonna

Thanks @divijvaidya and @clolov !

LGTM!

Once the builds are acceptable and I verified that they run the tests written in JUnit 5, I will merge this PR.

cadonna · 2022-07-29T15:13:46Z

I verified that all Streams' tests are run in the builds and I also verified that the tests of other modules are run in the builds.

cadonna · 2022-07-29T15:14:05Z

Build failures are unrelated.

…(5 August 2022) Version related conflicts: * Jenkinsfile * gradle.properties * streams/quickstart/java/pom.xml * streams/quickstart/java/src/main/resources/archetype-resources/pom.xml * streams/quickstart/pom.xml * tests/kafkatest/__init__.py * tests/kafkatest/version.py * commit 'add7cd85baa61cd0e1430': (66 commits) KAFKA-14136 Generate ConfigRecord for brokers even if the value is unchanged (apache#12483) HOTFIX / KAFKA-14130: Reduce RackAwarenesssTest to unit Test (apache#12476) MINOR: Remove ARM/PowerPC builds from Jenkinsfile (apache#12380) KAFKA-14111 Fix sensitive dynamic broker configs in KRaft (apache#12455) KAFKA-13877: Fix flakiness in RackAwarenessIntegrationTest (apache#12468) KAFKA-14129: KRaft must check manual assignments for createTopics are contiguous (apache#12467) KAFKA-13546: Do not fail connector validation if default topic creation group is explicitly specified (apache#11615) KAFKA-14122: Fix flaky test DynamicBrokerReconfigurationTest#testKeyStoreAlter (apache#12452) MINOR; Use right enum value for broker registration change (apache#12236) MINOR; Synchronize access to snapshots' TreeMap (apache#12464) MINOR; Bump trunk to 3.4.0-SNAPSHOT (apache#12463) MINOR: Stop logging 404s at ERROR level in Connect KAFKA-14095: Improve handling of sync offset failures in MirrorMaker (apache#12432) Minor: enable index for emit final sliding window (apache#12461) MINOR: convert some more junit tests to support KRaft (apache#12456) KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run (apache#12441) MINOR: Remove code of removed metric (apache#12453) MINOR: Update comment on verifyTaskGenerationAndOwnership method in DistributedHerder KAFKA-14012: Add warning to closeQuietly documentation about method references of null objects (apache#12321) MINOR: Fix static mock usage in ThreadMetricsTest (apache#12454) ...

Ensure both JUnit 4 and JUnit 5 tests run

5900bb6

clolov mentioned this pull request Jul 26, 2022

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

Merged

3 tasks

ijuma reviewed Jul 26, 2022

View reviewed changes

cadonna reviewed Jul 28, 2022

View reviewed changes

Run all streams test

e5faaf9

divijvaidya added 2 commits July 29, 2022 13:08

Revert "Run all streams test"

c6f9ab1

This reverts commit e5faaf9.

Run all streams test

bb05b35

divijvaidya reviewed Jul 29, 2022

View reviewed changes

cadonna approved these changes Jul 29, 2022

View reviewed changes

Fix checkstyle

899a34b

cadonna merged commit 54af64c into apache:trunk Jul 29, 2022

clolov deleted the KAFKA-7342-ENABLE-TESTS branch January 27, 2026 11:37

Conversation

clolov commented Jul 26, 2022

Uh oh!

divijvaidya commented Jul 26, 2022

Uh oh!

cadonna commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divijvaidya commented Jul 28, 2022

Uh oh!

cadonna commented Jul 28, 2022

Uh oh!

cadonna commented Jul 28, 2022

Uh oh!

divijvaidya commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Results

Before

After

Note

Comments

Next steps (separate PRs, in order)

Uh oh!

cadonna commented Jul 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divijvaidya commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Result for ./gradlew :streams:integrationTest

Before

After

Result for ./gradlew :streams:unitTest

Before

After

Uh oh!

cadonna commented Jul 29, 2022

Uh oh!

cadonna left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

divijvaidya commented Jul 29, 2022 •

edited

Loading

divijvaidya commented Jul 29, 2022 •

edited

Loading

Result for `./gradlew :streams:integrationTest`

Result for `./gradlew :streams:unitTest`

cadonna left a comment •

edited

Loading