divijvaidya commented on code in PR #12285:
URL: https://github.com/apache/kafka/pull/12285#discussion_r898982426
##########
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##########
@@ -236,6 +238,16 @@ public static String safeUniqueTestName(final Class<?>
testClass, final TestName
.replace('=', '_');
}
+ public static String safeUniqueTestName(final Class<?> testClass, final
TestInfo testInfo) {
Review Comment:
please add documentation
##########
streams/src/test/java/org/apache/kafka/streams/integration/RestoreIntegrationTest.java:
##########
@@ -87,37 +85,34 @@
import static
org.apache.kafka.streams.integration.utils.IntegrationTestUtils.waitForStandbyCompletion;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertTrue;
-@Category({IntegrationTest.class})
+@Timeout(600)
+@Tag("integration")
Review Comment:
I understand that this change is necessary for JUnit5 migration but in it's
current state if we merge this PR, these tests will not execute. Here's why:
In `build.gradle`, these tests are marked for execution with JUnit5 and
hence will enter the `else` block in
```
if (shouldUseJUnit5) {
useJUnitPlatform {
includeTags "integration"
}
} else {
useJUnit {
includeCategories 'org.apache.kafka.test.IntegrationTest'
}
}
```
where they will try to find the previous
`@Category({IntegrationTest.class})` but will not find it and hence, these
tests will not execute.
My suggestion would be to keep `@Category({IntegrationTest.class})` in these
tests and make all other changes.
Once we have all tests migrated to Junit5, create a PR where we remove the
Junit5 eligibility from `build.gradle` and at the same time, we replace this
annotation with `@Tag("integration")`
##########
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##########
@@ -236,6 +238,16 @@ public static String safeUniqueTestName(final Class<?>
testClass, final TestName
.replace('=', '_');
}
+ public static String safeUniqueTestName(final Class<?> testClass, final
TestInfo testInfo) {
Review Comment:
We can refactor the existing `safeUniqueTestName` as follows to extract out
common parts such as:
```
public static String safeUniqueTestName(final Class<?> testClass, final
TestName testName) {
return IntegrationTestUtils.safeUniqueTestName(testClass,
testName.getMethodName())
}
public static String safeUniqueTestName(final Class<?> testClass, final
TestInfo testInfo) {
return IntegrationTestUtils.safeUniqueTestName(testClass,
testInfo.getTestMethod().map(Method::getName))
}
private static String safeUniqueTestName(final Class<?> testClass, final
String methodName) {
...
}
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
