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.

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

[FLINK-30815][tests] Migrate BatchAbstractTestBase and BatchTestBase to junit5 #22427

Closed
wants to merge 3 commits into from

Conversation

TanYuxin-tyx
Copy link
Contributor

What is the purpose of the change

Migrate BatchAbstractTestBase and BatchTestBase to junit5

Brief change log

  • Migrate BatchAbstractTestBase and BatchTestBase to junit5

Verifying this change

This change is a test update without any test coverage.

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)

@TanYuxin-tyx TanYuxin-tyx changed the title Flink 30815 [FLINK-30815][tests] Migrate BatchAbstractTestBase and BatchTestBase to junit5 Apr 19, 2023
@flinkbot
Copy link
Collaborator

flinkbot commented Apr 19, 2023

CI report:

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

Copy link
Contributor

@JunRuiLee JunRuiLee left a comment

Choose a reason for hiding this comment

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

@TanYuxin-tyx Thank you for creating this PR. IIUC, this PR is to migrate BatchTestBase and BatchAbstractTestBase and their subclasses to Junit5 to avoid hide some problems. I noticed that not all subclasses are included in the current commits, such as SortITCase and DeleteTableITCase. Could you please help to check which other subclasses are still using Junit4 and make the necessary modifications? Thank you very much.

@TanYuxin-tyx
Copy link
Contributor Author

@JunRuiLee Thanks for joining the review. Considering that the two base classes have many table-related and Scala-related subclasses, it is indeed necessary to upgrade all the subclasses to JUnit5. However, I don't believe it is appropriate to update all the tests in this PR. Perhaps we should seek input from others to generate more ideas.

@godfreyhe could you please help take a look at this change, WDYT?

@JunRuiLee
Copy link
Contributor

@TanYuxin-tyx Thanks for the explanation and I agree that migrating all the subclasses in one PR is cumbersome and unnecessary. What confuses me is that the title of this PR is "Migrate BatchAbstractTestBase and BatchTestBase to junit5", but the commits also include migration of some subclasses, although not all of them. I am curious why these subclasses were selected for migration. Could you please clarify? Also, I suggest splitting this into different commits.

@TanYuxin-tyx
Copy link
Contributor Author

@JunRuiLee I agree that the sub subclasses migration can be in separate commits.
The reason for migrating some subclasses is that these classes have some problems in the new JUnit5 and can not run successfully. For example, TimestampITCase lacks the @Before annotation, which does not affect the JUnit4 test, but it can not run in the new JUnit5.

@TanYuxin-tyx
Copy link
Contributor Author

TanYuxin-tyx commented Apr 27, 2023

I don't use a separate commit to fix TimestampITCase finally, because separating to a new commit will cause the previous commit of Migrate BatchTestBase to junit5 can not run the test successfully, which does not comply with the principle that every commit can be passed independently.

In addition, other normal subclasses migrations should use separate commits.

Copy link
Contributor

@godfreyhe godfreyhe 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 the contribution, I left one minor comment

@TanYuxin-tyx
Copy link
Contributor Author

@godfreyhe Thanks for helping review the change. I have updated the code according to the comments.
I add 3 methods in BatchAbstractTestBase, createTempFolder, createTempFile, createFileInTempFolder. The subclasses call the appropriate method when needed.

Copy link
Contributor

@godfreyhe godfreyhe 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 the update, LGTM

@TanYuxin-tyx
Copy link
Contributor Author

@godfreyhe @JunRuiLee Thanks for helping review the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants