[GitHub] [flink] XComp commented on a diff in pull request #21289: [FLINK-29452] Allow unit tests to be executed independently

["XComp (via GitHub)" <gi...@apache.org>]

XComp commented on code in PR #21289:
URL: https://github.com/apache/flink/pull/21289#discussion_r1297093524


##########
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/junit/RetryOnFailureExtensionTest.java:
##########
@@ -21,56 +21,69 @@
 import org.apache.flink.testutils.junit.extensions.retry.RetryExtension;
 
 import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.ExtendWith;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import java.util.HashMap;
+
+import static org.assertj.core.api.Assertions.assertThat;
 
 /** Tests for the {@link RetryOnFailure} annotation on JUnit5 {@link RetryExtension}. */
 @ExtendWith(RetryExtension.class)
 class RetryOnFailureExtensionTest {
 
-    private static final int NUMBER_OF_RUNS = 5;
-
-    private static int numberOfFailedRuns;
+    private static final int NUMBER_OF_RETRIES = 5;
 
-    private static int numberOfSuccessfulRuns;
+    private static final HashMap<String, Integer> methodRunCount = new HashMap<>();
 
-    private static boolean firstRun = true;
+    @BeforeEach
+    void incrementMethodRunCount(TestInfo testInfo) {
+        // Set or increment the run count for the unit test method, by the method short name.
+        // This starts at 1 and is incremented before the test starts.
+        testInfo.getTestMethod()
+                .ifPresent(
+                        method ->
+                                methodRunCount.compute(
+                                        method.getName(), (k, v) -> (v == null) ? 1 : v + 1));
+    }
 
     @AfterAll
     static void verify() {
-        assertEquals(NUMBER_OF_RUNS + 1, numberOfFailedRuns);
-        assertEquals(3, numberOfSuccessfulRuns);
+        assertThat(methodRunCount.get("testRetryOnFailure"))

Review Comment:
   I think we can apply the same approach that I described above here. 



##########
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/junit/RetryOnExceptionExtensionTest.java:
##########
@@ -37,53 +40,66 @@
 @ExtendWith(RetryExtension.class)
 class RetryOnExceptionExtensionTest {
 
-    private static final int NUMBER_OF_RUNS = 3;
+    private static final int NUMBER_OF_RETRIES = 3;
 
-    private static int runsForSuccessfulTest = 0;
+    private static final HashMap<String, Integer> methodRunCount = new HashMap<>();
 
-    private static int runsForTestWithMatchingException = 0;
-
-    private static int runsForTestWithSubclassException = 0;
-
-    private static int runsForPassAfterOneFailure = 0;
+    @BeforeEach
+    void incrementMethodRunCount(TestInfo testInfo) {
+        // Set or increment the run count for the unit test method, by the method short name.
+        // This starts at 1 and is incremented before the test starts.
+        testInfo.getTestMethod()
+                .ifPresent(
+                        method ->
+                                methodRunCount.compute(
+                                        method.getName(), (k, v) -> (v == null) ? 1 : v + 1));
+    }
 
     @AfterAll
     static void verify() {
-        assertThat(runsForTestWithMatchingException).isEqualTo(NUMBER_OF_RUNS + 1);
-        assertThat(runsForTestWithSubclassException).isEqualTo(NUMBER_OF_RUNS + 1);
-        assertThat(runsForSuccessfulTest).isOne();
-        assertThat(runsForPassAfterOneFailure).isEqualTo(2);
+        assertThat(methodRunCount.get("testSuccessfulTest"))

Review Comment:
   I'm wondering whether we could also implement this test without having plain method names that we would have to add everywhere. We could do that by utilizing the `TestInfo` parameter in every test method and register the callbacks for the corresponding test method. That has the advantage that we have the test code in one location (the verification callback is defined within the test method instead of the `@AfterAll` annotated method). WDYT?
   
   We would have something like a `verificationCallbackRegistry` next to the `methodRunCount` and utilize methods like the following ones:
   ```
       private static int assertAndReturnRunCount(TestInfo testInfo) {
           return methodRunCount.get(assertAndReturnTestMethodName(testInfo));
       }
   
       private static void registerCallbackForTest(TestInfo testInfo, Consumer<Integer> verification) {
           verificationCallbackRegistry.putIfAbsent(
                   assertAndReturnTestMethodName(testInfo),
                   () -> verification.accept(assertAndReturnRunCount(testInfo)));
       }
   
       private static String assertAndReturnTestMethodName(TestInfo testInfo) {
           return testInfo.getTestMethod()
                   .orElseThrow(() -> new AssertionError("No test method is provided."))
                   .getName();
       }
   ```
   
   The testInfo comes from the test method's parameters. All test handling can happen within the test method. WDYT?



-- 
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: issues-unsubscribe@flink.apache.org

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