Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FLINK-24627][tests] add some Junit5 extensions to replace the existed Junit4 rules #17556

Merged
merged 1 commit into from Nov 24, 2021
Merged

[FLINK-24627][tests] add some Junit5 extensions to replace the existed Junit4 rules #17556

merged 1 commit into from Nov 24, 2021

Conversation

ruanhang1993
Copy link
Contributor

What is the purpose of the change

This pull request provides some junit5 extensions to replace the existed junit4 rules.

Brief change log

  • Add corresponding extensions for the existed junit4 rules. The extensions are named in a format of xxxxExtension.java, and replace the word Resource to Extension if the rule's filename is like xxxResource.java, e.g. MiniClusterResource.java corresponds to MiniClusterExtension.java.
  • Add CustomExtension, AllCallbackWrapper and EachCallbackWrapper for the rules that is both used as @Rule and @ClassRule.
  • Add retry extension in junit5, whose behavior is different from the RetryRule.
  • Add TestLoggerExtension to replace TestLogger

Verifying this change

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

Change Details

1. How to change junit4 rules to junit5 extension

A TestRule could be used as a @Rule or a @ClassRule in Junit4. We could make use of BeforeEachCallback and AfterEachCallback to rewrite @Rule, and use BeforeAllCallback and AfterAllCallback to rewrite @ClassRule.

So if the TestRule is only used as a @Rule, the new extension class will implement BeforeEachCallback and AfterEachCallback. if the TestRule is only used as a @ClassRule, the new extension class will implement BeforeAllCallback and AfterAllCallback.

There are some rules used both as a @Rule and a @ClassRule in flink. But a extension in junit5 can not implement both afterCallback and eachCallback, which will lead to some odd behaviors.

So if a TestRule is used both as a @Rule and a @ClassRule, the new extension should implement the provided CustomExtension interface. And it should be used with a AllCallbackWrapper or EachCallbackWrapper, and annotated with @RegisterExtension. A use case has already provided in flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskCancelAsyncProducerConsumerITCase.java.

2. The changed behavior in RetryExtension compared to RetryRule

If we want to execute a test multiple times in Junit5, we need implement TestTemplateInvocationContextProvider and annotate a test by @TestTemplate. And use a TestExecutionExceptionHandler to decide a test whether should be executed.

In this way, the number of retry tests in Junit5 is different from junit4. Because retry tests in Junit5 will plan to do test multiple times at the beginning, it will skip the following tests if has reached some condition, like succeed or throwing some unexpected exception.

@flinkbot
Copy link
Collaborator

flinkbot commented Oct 25, 2021 •
edited

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@flinkbot
Copy link
Collaborator

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 389f0b4 (Mon Oct 25 07:24:57 UTC 2021)

Warnings:

  • No documentation files were touched! Remember to keep the Flink docs up to date!
  • This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@zentol
Copy link
Contributor

zentol commented Oct 25, 2021

All the new files are missing the apache license header.

@ruanhang1993
Copy link
Contributor Author

All the new files are missing the apache license header.

Thanks, I will fix it.

@ruanhang1993
Copy link
Contributor Author

@flinkbot run azure

@zentol zentol self-assigned this Nov 10, 2021
zentol
zentol requested changes Nov 10, 2021
View reviewed changes
...s-parent/flink-test-utils-junit/src/main/java/org/apache/flink/util/TestLoggerExtension.java Show resolved Hide resolved
...-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/SharedObjectsExtension.java Show resolved Hide resolved
...-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/SharedObjectsExtension.java Show resolved Hide resolved
...ils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/util/LogLevelExtension.java Show resolved Hide resolved
...link-test-utils-junit/src/main/java/org/apache/flink/core/testutils/EachCallbackWrapper.java Outdated Show resolved Hide resolved
...-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryTestExecutionExtension.java Outdated Show resolved Hide resolved
...-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryTestExecutionExtension.java Outdated
public void handleTestExecutionException(ExtensionContext context, Throwable throwable)
throws Throwable {
String method = getTestMethodKey(context);
if (repeatableException != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any limitation as to what we can add to the ExtensionContext store? It would be quite neat if we could just encapsulate the 2 restart mechanisms in 2 implementations of a sort of restart strategy, and we just store one such instance in the store.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to refactor it , it seems not to have this limit.

...-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryTestExecutionExtension.java Outdated
"Retry test %s[%d/%d] failed with repeatable exception, continue retrying.",
method, retryIndex, totalTimes);
LOG.error(retryMsg, throwable);
throw new TestAbortedException(retryMsg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this result in a retry? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I do not find a way to control the execution of test cases except ExecutionCondition, the behavior of RetryExtension is different from RetryRule.
When use the RetryExtension, the test case will be planned to execute fixed times as the times in RetryOnException and RetryOnFailure. We could control which attempt to skip by ExecutionCondition. When we throw TestAbortedException for a test case, this test case execution will not be marked as failed, but as ignored.
Compare the test result of RetryOnExceptionExtensionTest(RetryExtension) and RetryOnExceptionTest(RetryRule), the difference could be seen more clearly.

...-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryTestExecutionExtension.java Outdated Show resolved Hide resolved
...-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryTestExecutionExtension.java Outdated Show resolved Hide resolved
@zentol
Copy link
Contributor

zentol commented Nov 15, 2021

There's a compile issue:

[ERROR] /__w/1/s/flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterWithClientExtension.java:[77,9] cannot find symbol
  symbol:   variable log
  location: class org.apache.flink.test.util.MiniClusterWithClientExtension
[ERROR] /__w/1/s/flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterWithClientExtension.java:[106,13] cannot find symbol
  symbol:   variable log
  location: class org.apache.flink.test.util.MiniClusterWithClientExtension

@ruanhang1993
Copy link
Contributor Author

@zentol I have fixed it, thanks for reminding.

zentol
zentol reviewed Nov 16, 2021
View reviewed changes
.../src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraConnectorITCase.java Outdated
import org.apache.flink.testutils.junit.RetryRule;
import org.apache.flink.testutils.junit.extensions.retry.annotation.RetryOnException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should revert this change. It can just easily lead to issues during rebases/backports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These annotations are used by Junit4 and Junit5 tests before we totally migrate to Junit5. Should I put them back to the package they used to be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes; that's what I meant.

zentol
zentol approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1; once CI is green I'll merge it!

@zentol
Copy link
Contributor

zentol commented Nov 23, 2021

@flinkbot run azure

@zentol zentol merged commit 78b231f into apache:master Nov 24, 2021
@RyanSkraba
Copy link
Contributor

RyanSkraba commented Sep 9, 2022 •
edited

Hello -- I've been looking at FLINK-29198. Is there any reason in this PR why the behaviour of @RetryOnException was changed to abort the test after too many retries throwing the expected exception? It looks like it was done explicitly!

Aborted tests are noted as skipped, and CI will pass without drawing attention to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snuyanzin snuyanzin snuyanzin left review comments

@zentol zentol zentol approved these changes

Assignees

@zentol zentol

Labels
component=Tests
Projects
None yet
Milestone
No milestone
6 participants
@ruanhang1993 @flinkbot @zentol @RyanSkraba @snuyanzin @rmetzger