XComp commented on code in PR #20805:
URL: https://github.com/apache/flink/pull/20805#discussion_r982266603
##########
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/junit/RetryOnExceptionExtensionTest.java:
##########
@@ -42,10 +51,10 @@ class RetryOnExceptionExtensionTest {
@AfterAll
static void verify() {
Review Comment:
This test implementation is odd. It doesn't allow the execution of
individual tests. I've created
[FLINK-29198](https://issues.apache.org/jira/browse/FLINK-29198) to cover that
topic.
##########
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/extensions/retry/strategy/RetryOnExceptionStrategy.java:
##########
@@ -37,6 +40,12 @@ public RetryOnExceptionStrategy(
@Override
public void handleException(String testName, int attemptIndex, Throwable
throwable)
Review Comment:
Could we improve the JavaDoc for this method in `RetryStrategy`?
Essentially, `TestAbortedException` is used for aborting a current test retry
run. Any other exception will result in a failure of the current test run.
calling `stopFollowingAttempts()` will result in any succeeding retries to not
be executed (see
[RetryTestExecutionExtension:53](https://github.com/apache/flink/blob/78b231f60aed59061f0f609e0cfd659d78e6fdd5/flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/extensions/retry/RetryTestExecutionExtension.java#L53)).
##########
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/extensions/retry/strategy/RetryOnExceptionStrategy.java:
##########
@@ -37,6 +40,12 @@ public RetryOnExceptionStrategy(
@Override
public void handleException(String testName, int attemptIndex, Throwable
throwable)
throws Throwable {
+ // Failed when reach the total retry times
+ if (attemptIndex >= totalTimes) {
+ LOG.error("Test Failed at the last retry.", throwable);
+ throw throwable;
+ }
Review Comment:
I'm wondering whether we should move this code into `AbstractRetryStrategy`
instead of using the same code snippet in both implementing classes.
##########
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/junit/RetryOnExceptionExtensionTest.java:
##########
@@ -80,4 +89,41 @@ void testPassAfterOneFailure() {
throw new IllegalArgumentException();
}
}
+
+ @ParameterizedTest(name = "With {0} retries for {1}")
+ @MethodSource("retryTestProvider")
+ void testRetryFailsWithExpectedExceptionAfterNumberOfRetries(
+ final int numberOfRetries, final Throwable expectedException) {
Review Comment:
nit: What's the reasoning behind adding this parameter here? In the end, we
only want to check whether the retry is triggered properly. Therefore, not
using a parameter here but rather a local `final int numberOfRetries = 1`
variable would reduce the signature. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
