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-33637][table-planner][JUnit5 Migration] Introduce ArchTest to ban Junit 4 for module table-planner #23791

Merged
merged 4 commits into from
Nov 29, 2023
Merged

[FLINK-33637][table-planner][JUnit5 Migration] Introduce ArchTest to ban Junit 4 for module table-planner #23791

merged 4 commits into from
Nov 29, 2023

Conversation

Jiabao-Sun
Copy link
Contributor

What is the purpose of the change

[FLINK-33637][table-planner][JUnit5 Migration] Introduce ArchTest to ban Junit 4 for module table-planner

Brief change log

Currently, table-planner have already completed the migration to JUnit 5, and we need to prevent the introduction of JUnit 4 tests in new PRs.

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 documented)

@Jiabao-Sun
Copy link
Contributor Author

Hi @leonardBang, please help review it when you have time.
Thanks.

@leonardBang leonardBang self-requested a review November 24, 2023 09:22
@flinkbot
Copy link
Collaborator

flinkbot commented Nov 24, 2023 •
edited
Loading

CI report:

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

@leonardBang
Copy link
Contributor

Could you rebase to latest master ? @Jiabao-Sun

JingGe
JingGe reviewed Nov 24, 2023
View reviewed changes
Copy link
Contributor

@JingGe JingGe left a comment

Choose a reason for hiding this comment

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

Thanks for driving it. I just left a comment and look forward to your thoughts

...link-table-planner/src/test/java/org/apache/flink/architecture/TestCodeArchitectureTest.java
})
public class TestCodeArchitectureTest {

@ArchTest public static final ArchTests BAN_JUNIT4_TESTS = ArchTests.in(BanJunit4Rules.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to enable the ITCaseRules too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @JingGe for the review.
ITCaseRules was enabled and made some refactoring to meet the rules.
Please help review it again when you have time.

JingGe
JingGe approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@JingGe JingGe left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@JingGe
Copy link
Contributor

JingGe commented Nov 29, 2023 •
edited
Loading

Thanks @Jiabao-Sun for the update. There are some ArchUnit violation changes in other modules. Did you run the ArchUnit test in those modules explicitly?

@Jiabao-Sun
Copy link
Contributor Author

Thanks @Jiabao-Sun for the update. There are some ArchUnit violations in other modules. Did you run the ArchUnit test in those modules explicitly?

Thanks @JingGe for the review.
I did not explicitly run the ArchUnit test in other modules.
The violations reduction of the ArchUnit test of other modules in this PR is due to the repair of the Base classes in the flink-table-planner and there is no new violations that introduced by this PR.

@JingGe
Copy link
Contributor

JingGe commented Nov 29, 2023

Thanks @JingGe for the review. I did not explicitly run the ArchUnit test in other modules. The violations reduction of the ArchUnit test of other modules in this PR is due to the repair of the Base classes in the flink-table-planner and there is no new violations that introduced by this PR.

Makes sense. Thanks for the info. Would you like to squash commits and rebase?

JingGe
JingGe approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@JingGe JingGe left a comment

Choose a reason for hiding this comment

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

LGTM

leonardBang
leonardBang approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

LGTM

@Jiabao-Sun
Copy link
Contributor Author

squash is fine, thanks.

@leonardBang leonardBang merged commit b9f3b95 into apache:master Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JingGe JingGe JingGe approved these changes

@leonardBang leonardBang leonardBang approved these changes

Assignees

@Jiabao-Sun Jiabao-Sun

Labels
component=Tests
Projects
None yet
Milestone
No milestone
4 participants
@Jiabao-Sun @flinkbot @leonardBang @JingGe