[GitHub] [kafka] divijvaidya commented on a diff in pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1

[GitBox <gi...@apache.org>]

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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org