[FLINK-26252][runtime][test-utils] Refactor MiniClusterExtension to support JUnit 5 parallel tests #18847

slinkydeveloper · 2022-02-18T13:58:45Z

What is the purpose of the change

The goal of this PR is to fix MiniClusterWithClientExtension (renamed to MiniClusterExtension) to properly support parallel tests, to be used with the ExtendWith annotation and to correctly tie to the lifecycle when creating one cluster per class.

Brief change log

Reworked MiniClusterExtension to add support for parallel tests and injection of client as test parameters
Renamed MiniClusterExtension in flink-runtime to avoid confusion and misusages

flinkbot · 2022-02-18T14:02:12Z

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 7f3404d (Fri Feb 18 14:02:12 UTC 2022)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{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 · 2022-02-18T14:03:47Z

CI report:

1cfeaa7 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

...flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/AllCallbackWrapper.java

...s-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterExtension.java

...k-runtime/src/test/java/org/apache/flink/runtime/testutils/InternalMiniClusterExtension.java

...s-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterExtension.java

zentol · 2022-02-21T11:26:50Z

The architecture tests are complaining because they don't recognize the MiniClusterExtension as a mini cluster without the CallbackWrappers.

...ase/src/test/java/org/apache/flink/connector/base/source/reader/AlignedWatermarksITCase.java

...-connector-pulsar/src/test/java/org/apache/flink/connector/pulsar/sink/PulsarSinkITCase.java

...k-architecture-tests-test/src/main/java/org/apache/flink/architecture/rules/ITCaseRules.java

slinkydeveloper · 2022-02-21T17:05:06Z

@JingGe can you please check this out?

...k-connectors/flink-connector-aws-kinesis-data-streams/src/test/resources/archunit.properties

ruanhang1993 · 2022-02-22T15:20:04Z

@slinkydeveloper , Thanks for your work.
MiniClusterWithClientExtension is aimed to replace the Junit 4 rule MiniClusterWithClientResource for Junit 5 miguration. The same is the MiniClusterExtension for MiniClusterResource. Because of some unresolved discussions, these extensions are only used in one or two classes as an example.

The MiniClusterWithClientResource rule could be used as @Rule or @ClassRule. In order to resolve conflicts, we introduce each/all wrappers for Junit 5 extensions. Maybe you can search the usages of MiniClusterWithClientResource. I think it is better to adapt @Rule and @ClassRule in Junit 5 when refactoring.

To be honest, I don't understand why the MiniClusterResource is used as a @Rule to start and end a mini cluster for every test cases.

slinkydeveloper · 2022-02-22T15:27:45Z

I think it is better to adapt @rule and @ClassRule in Junit 5 when refactoring.
To be honest, I don't understand why the MiniClusterResource is used as a @rule to start and end a mini cluster for every test cases.

Exactly my point on why I didn't added this feature here. It doesn't seem there are much tests doing that, and probably most of the ones that are doing it, are doing it by mistake. I would rather not add the capability in this PR. If it's really needed, we add it in future, but now as default this extension always starts only one cluster per class. Another aspect is that if you really need to create a cluster per method, then just manually instantiating MiniCluster should be enough, and you don't need the extension doing it for you.

zentol · 2022-02-22T16:18:22Z

There are use-cases where it can make sense to run dedicated clusters for each test, for example when you add a taskmanager to the cluster within a test (test scale-up of a job), doing something destructive like killing a taskmanager (scala-down / recovery) or creating an intentional resource leak.
Mind you that that these things are easily possible since we expose the MiniCluster.

However, I'd consider these as niche use-cases that we shouldn't don't have to focus on (at this time). There are multiple ways to workaround this (spread tests across classes / wrap extension / manual life-cycling) in any case.

zentol

Running CI in the background; will update PR and merge once it's green.

JingGe

Thanks @slinkydeveloper for driving this PR. It looks overall good. I just left some comments.

JingGe · 2022-02-23T09:25:26Z

flink-architecture-tests/flink-architecture-tests-test/README.md

+
+```
+rm -rf `find . -type d -name archunit-violations`
+mvn test -Dtest="*TestCodeArchitectureTest*" -DfailIfNoTests=false -Darchunit.freeze.refreeze=true -Darchunit.freeze.store.default.allowStoreCreation=true -Dfast


NIT： -Dskip.npm=true

JingGe · 2022-02-23T09:27:14Z

...k-architecture-tests-test/src/main/java/org/apache/flink/architecture/rules/ITCaseRules.java

+     * <p>2. For JUnit 5 test, use {@link ExtendWith}:
+     *
+     * <pre>{@code
+     * @ExtendWith(MiniClusterExtension.class)


Where to use it? at class level?

JingGe · 2022-02-23T09:28:59Z

...k-architecture-tests-test/src/main/java/org/apache/flink/architecture/rules/ITCaseRules.java

@@ -135,14 +140,46 @@
                        MiniClusterWithClientResource.class, Rule.class));
    }

+    private static DescribedPredicate<JavaClass> inFlinkRuntimePackages() {
+        return JavaClass.Predicates.resideInAPackage("org.apache.flink.runtime.*");


import static?

JingGe · 2022-02-23T09:34:48Z

...k-runtime/src/test/java/org/apache/flink/runtime/testutils/InternalMiniClusterExtension.java

+/**
+ * An extension which starts a {@link MiniCluster} for testing purposes.
+ *
+ * <p>This should only be used by tests within the flink-runtime module. Other modules should use


Could you add more description to tell why a different MiniClusterExtension is required explicitly for flink-runtime and the same MiniClusterExtension can not be used by other modules? I think the idea of using @InjectMiniCluster is excellent and could be very useful for tests in other modules beyond flink-runtime too.

MiniClusterExtension requires flink-clients. InjectMiniCluster can also be used with the MiniClusterExtension.

Certain configuration parameters (e.g., ports) are inferred from other extensions. As extensions commonly only really start up resources in before*() methods the current approach of determining the configuration up-front imposes limitations as to whether a MiniClusterExtension can be static or not. With this change the configuration can also be passed in with a Supplier that is evaluated right before the cluster is started.

Refactors the test to make use of a static MiniClusterExtension, in preperation FLINK-26252.

… parallel tests

zentol self-assigned this Feb 18, 2022

zentol reviewed Feb 18, 2022

View reviewed changes

rmetzger added the component=TestInfrastructure label Feb 18, 2022

zentol reviewed Feb 21, 2022

View reviewed changes

...s-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterExtension.java Outdated Show resolved Hide resolved

zentol reviewed Feb 21, 2022

View reviewed changes

...ase/src/test/java/org/apache/flink/connector/base/source/reader/AlignedWatermarksITCase.java Outdated Show resolved Hide resolved

zentol reviewed Feb 21, 2022

View reviewed changes

...-connector-pulsar/src/test/java/org/apache/flink/connector/pulsar/sink/PulsarSinkITCase.java Outdated Show resolved Hide resolved

zentol reviewed Feb 21, 2022

View reviewed changes

...k-architecture-tests-test/src/main/java/org/apache/flink/architecture/rules/ITCaseRules.java Outdated Show resolved Hide resolved

slinkydeveloper force-pushed the FLINK-26252 branch from b1d7e49 to 3ce599b Compare February 21, 2022 17:04

slinkydeveloper force-pushed the FLINK-26252 branch from 3ce599b to 6dc0997 Compare February 22, 2022 08:41

slinkydeveloper mentioned this pull request Feb 22, 2022

[FLINK-26294][test] Using fixed description for ArchUnit rules #18867

Merged

zentol reviewed Feb 22, 2022

View reviewed changes

...k-connectors/flink-connector-aws-kinesis-data-streams/src/test/resources/archunit.properties Outdated Show resolved Hide resolved

slinkydeveloper force-pushed the FLINK-26252 branch 2 times, most recently from 9ce87b8 to 1cfeaa7 Compare February 22, 2022 14:48

zentol approved these changes Feb 22, 2022

View reviewed changes

JingGe reviewed Feb 23, 2022

View reviewed changes

zentol and others added 3 commits February 24, 2022 14:27

[hotfix][tests] Restructure AbstractHaJobRunITCase

19e3227

Refactors the test to make use of a static MiniClusterExtension, in preperation FLINK-26252.

[FLINK-26252][tests] Refactor MiniClusterExtension to support JUnit 5…

afd06b5

… parallel tests

zentol force-pushed the FLINK-26252 branch from 1cfeaa7 to afd06b5 Compare February 24, 2022 15:56

zentol merged commit 8cdd0b8 into apache:master Feb 24, 2022

slinkydeveloper deleted the FLINK-26252 branch March 1, 2022 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-26252][runtime][test-utils] Refactor MiniClusterExtension to support JUnit 5 parallel tests #18847

[FLINK-26252][runtime][test-utils] Refactor MiniClusterExtension to support JUnit 5 parallel tests #18847

slinkydeveloper commented Feb 18, 2022

flinkbot commented Feb 18, 2022

flinkbot commented Feb 18, 2022 •

edited

zentol commented Feb 21, 2022

slinkydeveloper commented Feb 21, 2022

ruanhang1993 commented Feb 22, 2022 •

edited

slinkydeveloper commented Feb 22, 2022

zentol commented Feb 22, 2022

zentol left a comment

JingGe left a comment

JingGe Feb 23, 2022

JingGe Feb 23, 2022

zentol Feb 24, 2022

JingGe Feb 23, 2022

JingGe Feb 23, 2022

zentol Feb 24, 2022

[FLINK-26252][runtime][test-utils] Refactor MiniClusterExtension to support JUnit 5 parallel tests #18847

[FLINK-26252][runtime][test-utils] Refactor MiniClusterExtension to support JUnit 5 parallel tests #18847

Conversation

slinkydeveloper commented Feb 18, 2022

What is the purpose of the change

Brief change log

flinkbot commented Feb 18, 2022

Automated Checks

Review Progress

flinkbot commented Feb 18, 2022 • edited

CI report:

zentol commented Feb 21, 2022

slinkydeveloper commented Feb 21, 2022

ruanhang1993 commented Feb 22, 2022 • edited

slinkydeveloper commented Feb 22, 2022

zentol commented Feb 22, 2022

zentol left a comment

Choose a reason for hiding this comment

JingGe left a comment

Choose a reason for hiding this comment

JingGe Feb 23, 2022

Choose a reason for hiding this comment

JingGe Feb 23, 2022

Choose a reason for hiding this comment

zentol Feb 24, 2022

Choose a reason for hiding this comment

JingGe Feb 23, 2022

Choose a reason for hiding this comment

JingGe Feb 23, 2022

Choose a reason for hiding this comment

zentol Feb 24, 2022

Choose a reason for hiding this comment

flinkbot commented Feb 18, 2022 •

edited

ruanhang1993 commented Feb 22, 2022 •

edited