[FLINK-23255][test] Introduce JUnit 5 dependencies #16551

PatrickRen · 2021-07-21T07:31:31Z

What is the purpose of the change

This pull request introduces JUnit 5 dependencies into Flink project so that developers can write JUnit 5 style test cases.

Brief change log

Add junit-jupiter dependency in root pom
Add junit-vintage-engine and junit-jupiter-migrationsupport dependency for supporting existing JUnit 4 test cases.

Verifying this change

This change is already covered by all existing tests.

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

Dependencies (does it add or upgrade a dependency): (yes)
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)

flinkbot · 2021-07-21T07:34:47Z

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 58f4488 (Wed Jul 21 07:34:46 UTC 2021)

Warnings:

11 pom.xml files were touched: Check for build and licensing issues.
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

flinkbot · 2021-07-21T08:00:36Z

CI report:

7866567 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

zentol · 2021-07-21T08:05:04Z

pom.xml

@@ -1578,7 +1591,7 @@ under the License.
 <plugin>
 <groupId>org.apache.maven.plugins</groupId>
 <artifactId>maven-surefire-plugin</artifactId>
- <version>2.22.1</version>
+ <version>2.22.2</version>


Why is this being bumped?

Thanks for the review @zentol! 2.22.2 fixed a bug related to junit-vintage-engine, so I bumped the version.

AHeise

I have verified that the test count is plausible, however from the AZP run, I could not assess if JUnit5 is actually used. The easiest way would be to temporarily add a test that fails and look at the stack trace.

AHeise · 2021-07-28T11:07:09Z

flink-end-to-end-tests/flink-end-to-end-tests-common-kafka/pom.xml

+
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-migrationsupport</artifactId>


Do we even need this? Afaik this is for using JUnit4 rules in JUnit5 tests.
I had expected instead to see the vintage runner.

Same comment for the other modules.

PatrickRen · 2021-08-03T13:50:12Z

Thanks for the review @AHeise !

Sorry for misunderstanding the usage of junit-jupiter-migrationsupport. I have removed them in the latest commit.

About validating whether Jupiter engine is truly used, I tried to check the stack trace but found that the stack is different under different execution environments. For example Intellij IDEA has its own engine so I can't capture keyword "jupiter" in the stack trace. Instead I added a temp test class JUnitJupiterTest with JUnit5 unique features. I think this case can prove that Jupiter engine truly works in the project.

AHeise · 2021-08-04T11:38:30Z

Sorry for misunderstanding the usage of junit-jupiter-migrationsupport. I have removed them in the latest commit.

Yes but I think you need to add vintage at some of those places (migrationsupport has it as a transitive dependency afaik) and now JUnit4 classes are missing.

About validating whether Jupiter engine is truly used, I tried to check the stack trace but found that the stack is different under different execution environments. For example Intellij IDEA has its own engine so I can't capture keyword "jupiter" in the stack trace. Instead I added a temp test class JUnitJupiterTest with JUnit5 unique features. I think this case can prove that Jupiter engine truly works in the project.

Sounds good to me and is a neat idea. We can double-check if the test count is the same and your new test appears on AZP and then this PR is good to go.

PatrickRen · 2021-08-06T15:37:10Z

@AHeise Thanks again for the review! I made a rebase to sync with the latest master.

I manually ran CI twice in my own AZP project, one with the JUnit 5 dependencies[1] and another without[2]. Number of tests are almost identical to each other (59,186 vs 59,189) only with slight diffs.

Missing tests with JUnit5 dependencies:

org.apache.flink.testutils.junit.RetryRuleTest$TestClassWithAnnotation
org.apache.flink.testutils.junit.RetryRuleTest$TestClassWithAnnotationOnMethod
org.apache.flink.testutils.junit.RetryRuleTest$TestClassWithTestExpectingRuntimeException
org.apache.flink.testutils.junit.RetryRuleTest$TestClassWithoutAnnotation
org.apache.flink.test.checkpointing.LocalRecoveryITCase$EventTimeWindowCheckpointingITCaseInstance
org.apache.flink.api.common.typeutils.CompositeSerializerTest$CompositeSerializerTestInstance
org.apache.flink.api.java.typeutils.runtime.AbstractGenericTypeComparatorTest$ComparatorTestInstance
org.apache.flink.api.java.typeutils.runtime.EitherSerializerTest$EitherSerializerTestInstance
org.apache.flink.api.java.typeutils.runtime.LegacyRowSerializerTest$RowSerializerTestInstance
org.apache.flink.api.java.typeutils.runtime.RowSerializerTest$RowSerializerTestInstance

Interesting thing is that all these tests are annotated with @Ignore. Not sure why these cases are run in JUnit4.

Additional tests with JUnit5 dependencies:

org.apache.flink.test.junit5.JUnitJupiterTest * 6 (Intentionally added for testing Jupiter engine)
org.apache.flink.connector.jdbc.xa.JdbcXaSinkMigrationTest#testCommitFromSnapshot (Not sure why this case is not run in JUnit4)

In general, I think all test cases are covered with by vintage engine, and new Jupiter engine is validated by JUnitJupiterTest.

[1] With JUnit 5: https://dev.azure.com/renqs/Apache%20Flink/_build/results?buildId=42&view=ms.vss-test-web.build-test-results-tab
[2] Without JUnit 5: https://dev.azure.com/renqs/Apache%20Flink/_build/results?buildId=43&view=ms.vss-test-web.build-test-results-tab

AHeise

LGTM.
I'm assuming that one additional test was ignored on master or whatever you compared with.
Let me know if I should merge. It would be best to do it at a time when you can also check if it causes issues with subsequent builds.

[FLINK-23255][test] Introduce JUnit 5 dependencies. All existing JUnit4 tests are run through the vintage runner of JUnit5.

)" This reverts commit 803fb8c.

[FLINK-23255][test] Introduce JUnit 5 dependencies. All existing JUnit4 tests are run through the vintage runner of JUnit5.

rmetzger added the review=description? label Jul 21, 2021

zentol reviewed Jul 21, 2021

View reviewed changes

rmetzger added the component=Tests label Jul 21, 2021

AHeise reviewed Jul 28, 2021

View reviewed changes

AHeise self-assigned this Jul 29, 2021

PatrickRen force-pushed the FLINK-23255 branch from 58f4488 to eb42585 Compare August 3, 2021 13:41

PatrickRen added 2 commits August 6, 2021 16:44

[FLINK-23255][test] Introduce JUnit 5 dependencies

7c9b589

[FLINK-23255][test] Add vintage engine for test util modules

7866567

PatrickRen force-pushed the FLINK-23255 branch from d2b7598 to 7866567 Compare August 6, 2021 08:44

AHeise approved these changes Aug 6, 2021

View reviewed changes

AHeise mentioned this pull request Aug 10, 2021

[FLINK-19554][connector/testing-framework] Connector Testing Framework #16590

Merged

AHeise merged commit f8ce4d7 into apache:master Aug 10, 2021

hhkkxxx133 pushed a commit to hhkkxxx133/flink that referenced this pull request Aug 25, 2021

[FLINK-23255][test] Introduce JUnit 5 dependencies (apache#16551)

ecf53aa

[FLINK-23255][test] Introduce JUnit 5 dependencies. All existing JUnit4 tests are run through the vintage runner of JUnit5.

gddezero pushed a commit to gddezero/flink that referenced this pull request Feb 17, 2023

[FLINK-23255][test] Introduce JUnit 5 dependencies (apache#16551)

803fb8c

[FLINK-23255][test] Introduce JUnit 5 dependencies. All existing JUnit4 tests are run through the vintage runner of JUnit5.

gddezero added a commit to gddezero/flink that referenced this pull request Feb 17, 2023

Revert "[FLINK-23255][test] Introduce JUnit 5 dependencies (apache#16551

d1cbb95

)" This reverts commit 803fb8c.

jnh5y pushed a commit to jnh5y/flink that referenced this pull request Dec 18, 2023

[FLINK-23255][test] Introduce JUnit 5 dependencies (apache#16551)

17ec465

[FLINK-23255][test] Introduce JUnit 5 dependencies. All existing JUnit4 tests are run through the vintage runner of JUnit5.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-23255][test] Introduce JUnit 5 dependencies #16551

[FLINK-23255][test] Introduce JUnit 5 dependencies #16551

PatrickRen commented Jul 21, 2021

flinkbot commented Jul 21, 2021

flinkbot commented Jul 21, 2021 •

edited

Loading

zentol Jul 21, 2021

PatrickRen Jul 21, 2021

AHeise left a comment

AHeise Jul 28, 2021

PatrickRen commented Aug 3, 2021

AHeise commented Aug 4, 2021 •

edited

Loading

PatrickRen commented Aug 6, 2021

AHeise left a comment

[FLINK-23255][test] Introduce JUnit 5 dependencies #16551

[FLINK-23255][test] Introduce JUnit 5 dependencies #16551

Conversation

PatrickRen commented Jul 21, 2021

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

flinkbot commented Jul 21, 2021

Automated Checks

Review Progress

flinkbot commented Jul 21, 2021 • edited Loading

CI report:

zentol Jul 21, 2021

Choose a reason for hiding this comment

PatrickRen Jul 21, 2021

Choose a reason for hiding this comment

AHeise left a comment

Choose a reason for hiding this comment

AHeise Jul 28, 2021

Choose a reason for hiding this comment

PatrickRen commented Aug 3, 2021

AHeise commented Aug 4, 2021 • edited Loading

PatrickRen commented Aug 6, 2021

AHeise left a comment

Choose a reason for hiding this comment

flinkbot commented Jul 21, 2021 •

edited

Loading

AHeise commented Aug 4, 2021 •

edited

Loading