You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/07/26 12:38:48 UTC
[GitHub] [kafka] clolov opened a new pull request, #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
clolov opened a new pull request, #12441:
URL: https://github.com/apache/kafka/pull/12441
This pull request addresses the problem reported in https://github.com/apache/kafka/pull/12285 and tracked in https://issues.apache.org/jira/browse/KAFKA-14108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1199094555
## Changes
1. Replace with `@Category({IntegrationTest.class})` with `@Tag("integration")`
2. Unblock `streams` project from using JUnitPlatform.
3. 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`.
4. Minor change required in `KTableSourceTopicRestartIntegrationTest.java` after using `junit-jupiter`
## Results
Result of executing `./gradlew :streams:test` before & after this change.
### Before
<img width="1296" alt="Screenshot 2022-07-29 at 11 30 16" src="https://user-images.githubusercontent.com/71267/181733713-2dff4282-13b3-4359-96d3-7604cb501a59.png">
### After
<img width="1286" alt="Screenshot 2022-07-29 at 11 20 44" src="https://user-images.githubusercontent.com/71267/181733751-5c86103a-146a-4b4d-8a58-0a77b2395abc.png">
Note that 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.
## 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)
1. Migrate tests using PowerMock to use Mockito
2. Migrate all tests to Junit5
3. Remove the Junit4 dependency completely from `build.gradle`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1199112880
@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 https://github.com/apache/kafka/pull/12441#discussion_r932429697.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1195488452
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().
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r932248380
##########
gradle/dependencies.gradle:
##########
@@ -159,6 +159,10 @@ libs += [
jmhGeneratorAnnProcess: "org.openjdk.jmh:jmh-generator-annprocess:$versions.jmh",
joptSimple: "net.sf.jopt-simple:jopt-simple:$versions.jopt",
jose4j: "org.bitbucket.b_c:jose4j:$versions.jose4j",
+ // 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",
Review Comment:
That makes sense. There should be no need for depending on the old junit4 jars directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1199195289
Updated the code. @cadonna should be ready for your review ones the test run is complete.
## Result for `./gradlew :streams:integrationTest`
### Before
### After
## Result for `./gradlew :streams:unitTest`
### Before
### After
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r929998599
##########
gradle/dependencies.gradle:
##########
@@ -159,6 +159,10 @@ libs += [
jmhGeneratorAnnProcess: "org.openjdk.jmh:jmh-generator-annprocess:$versions.jmh",
joptSimple: "net.sf.jopt-simple:jopt-simple:$versions.jopt",
jose4j: "org.bitbucket.b_c:jose4j:$versions.jose4j",
+ // 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",
Review Comment:
This explanation is odd - compilation works currently and we don't have this dependency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1198365454
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933076700
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
useJUnitPlatform {
includeTags "integration"
+ // KAFKA-14109
Review Comment:
Addressed the comment in the latest revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna merged pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna merged PR #12441:
URL: https://github.com/apache/kafka/pull/12441
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r929996624
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
useJUnitPlatform {
includeTags "integration"
+ // KAFKA-14109
Review Comment:
We don't typically include jira references in the code like this (it pollutes the code and goes stale typically).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r932429697
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
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"
}
Review Comment:
I experimented a bit and found the following solution.
```suggestion
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.
##########
build.gradle:
##########
@@ -1832,12 +1840,17 @@ project(':streams') {
// testCompileOnly prevents streams from exporting a dependency on test-utils, which would cause a dependency cycle
testCompileOnly project(':streams:test-utils')
+ // 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
+
Review Comment:
As we said below this is not needed if you move `libs.junitVintageEngine` back to `testImplementation`.
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
useJUnitPlatform {
includeTags "integration"
+ // KAFKA-14109
Review Comment:
I second Ismael's comment.
##########
build.gradle:
##########
@@ -503,6 +507,10 @@ subprojects {
if (shouldUseJUnit5) {
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"
}
Review Comment:
Here we can do similar as above but excluding the tags.
```suggestion
if (project.name.equals('streams')) {
useJUnitPlatform {
excludeTags "integration"
excludeTags 'org.apache.kafka.test.IntegrationTest'
includeEngines "junit-vintage", "junit-jupiter"
}
} else {
useJUnitPlatform {
excludeTags "integration"
}
}
```
##########
build.gradle:
##########
@@ -1832,12 +1840,17 @@ project(':streams') {
// testCompileOnly prevents streams from exporting a dependency on test-utils, which would cause a dependency cycle
testCompileOnly project(':streams:test-utils')
+ // 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
+
testImplementation project(':clients').sourceSets.test.output
testImplementation project(':core')
testImplementation project(':core').sourceSets.test.output
testImplementation libs.log4j
- testImplementation libs.junitJupiterApi
- testImplementation libs.junitVintageEngine
+ testImplementation libs.junitJupiter
+ testImplementation libs.junitJupiterParams // needed for parameterized tests
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r932242390
##########
gradle/dependencies.gradle:
##########
@@ -159,6 +159,10 @@ libs += [
jmhGeneratorAnnProcess: "org.openjdk.jmh:jmh-generator-annprocess:$versions.jmh",
joptSimple: "net.sf.jopt-simple:jopt-simple:$versions.jopt",
jose4j: "org.bitbucket.b_c:jose4j:$versions.jose4j",
+ // 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",
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933077282
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
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"
}
Review Comment:
Made the changes as suggested. Please see latest revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1199503553
Build failures are unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1198367979
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r930013162
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
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"
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r929996127
##########
build.gradle:
##########
@@ -466,6 +466,10 @@ subprojects {
if (shouldUseJUnit5) {
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"
Review Comment:
We don't want to do this for all modules since most of them have already been converted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933164909
##########
streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java:
##########
@@ -100,7 +98,7 @@ public static void closeCluster() {
@BeforeEach
public void before(final TestInfo testInfo) throws Exception {
- sourceTopic = SOURCE_TOPIC + "-" + testInfo.getTestMethod().map(Method::getName);
+ sourceTopic = SOURCE_TOPIC + "-" + IntegrationTestUtils.safeUniqueTestName(getClass(), testInfo);
Review Comment:
Note that the test fails without this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933169101
##########
streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java:
##########
@@ -100,7 +98,7 @@ public static void closeCluster() {
@BeforeEach
public void before(final TestInfo testInfo) throws Exception {
- sourceTopic = SOURCE_TOPIC + "-" + testInfo.getTestMethod().map(Method::getName);
+ sourceTopic = SOURCE_TOPIC + "-" + IntegrationTestUtils.safeUniqueTestName(getClass(), testInfo);
Review Comment:
Yeah, thank you! That is known. See https://github.com/apache/kafka/pull/12441#issuecomment-1195516666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933076952
##########
gradle/dependencies.gradle:
##########
@@ -159,6 +159,10 @@ libs += [
jmhGeneratorAnnProcess: "org.openjdk.jmh:jmh-generator-annprocess:$versions.jmh",
joptSimple: "net.sf.jopt-simple:jopt-simple:$versions.jopt",
jose4j: "org.bitbucket.b_c:jose4j:$versions.jose4j",
+ // 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",
Review Comment:
Removed the dependency in latest revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933077530
##########
build.gradle:
##########
@@ -1832,12 +1840,17 @@ project(':streams') {
// testCompileOnly prevents streams from exporting a dependency on test-utils, which would cause a dependency cycle
testCompileOnly project(':streams:test-utils')
+ // 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
+
Review Comment:
Removed this dependency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1199204438
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1198361209
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1195438950
@clolov
1. don't we need to remove "streams" from
```
def shouldUseJUnit5 = !(["runtime", "streams"].contains(it.project.name))
```
as well?
2. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1195516666
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12441:
URL: https://github.com/apache/kafka/pull/12441#discussion_r933077759
##########
build.gradle:
##########
@@ -1832,12 +1840,17 @@ project(':streams') {
// testCompileOnly prevents streams from exporting a dependency on test-utils, which would cause a dependency cycle
testCompileOnly project(':streams:test-utils')
+ // 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
+
testImplementation project(':clients').sourceSets.test.output
testImplementation project(':core')
testImplementation project(':core').sourceSets.test.output
testImplementation libs.log4j
- testImplementation libs.junitJupiterApi
- testImplementation libs.junitVintageEngine
+ testImplementation libs.junitJupiter
+ testImplementation libs.junitJupiterParams // needed for parameterized tests
Review Comment:
I have removed this right now to keep this PR simple. Will add when needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12441: KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12441:
URL: https://github.com/apache/kafka/pull/12441#issuecomment-1199502782
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org