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-32854][flink-runtime][JUnit5 Migration] The state package of flink-runtime module #23218

Merged
merged 8 commits into from
Sep 11, 2023

Conversation

Jiabao-Sun
Copy link
Contributor

[FLINK-32854][flink-runtime][JUnit5 Migration] The state package of flink-runtime module

What is the purpose of the change

[flink-runtime][JUnit5 Migration] The state package of flink-runtime module

Brief change log

[flink-runtime][JUnit5 Migration] The state package of flink-runtime module

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

@flinkbot
Copy link
Collaborator

flinkbot commented Aug 15, 2023

CI report:

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

@1996fanrui
Copy link
Member

1996fanrui commented Aug 16, 2023

Hi @ferenc-csaky , would you mind helping review this PR in your free time? thanks~

There are too many test migration PRs are waiting for my review, and I don't have enough time to do all review. It's better for contributors to review each other first. Of course, I will review them as well.

cc @Jiabao-Sun

@1996fanrui 1996fanrui self-requested a review August 16, 2023 13:16
Copy link
Contributor

@ferenc-csaky ferenc-csaky 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 making this! Added some comments.

@Jiabao-Sun
Copy link
Contributor Author

Thanks @ferenc-csaky for the detailed review and helpful suggestions.
Most of the comments are fixed.
Please help review it again when you have time.

Copy link
Contributor

@ferenc-csaky ferenc-csaky 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 improvements @Jiabao-Sun, added 1 more comment, other than that it LGTM!

Copy link
Contributor

@ferenc-csaky ferenc-csaky left a comment

Choose a reason for hiding this comment

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

Thanks @Jiabao-Sun LGTM!

@Jiabao-Sun
Copy link
Contributor Author

Hi @1996fanrui.
Could you help review this PR when you have time?

@1996fanrui
Copy link
Member

Hi @1996fanrui. Could you help review this PR when you have time?

Sure, all of your PRs that migrating the test from junit4 to junit5 of flink-runtime module are in my review list.

However, some of them are huge, and I'm busy recently. I will review them in my free time, thanks for the understand~

Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Hi @Jiabao-Sun , thanks for the contribution.

I didn't review this PR in detail, and just checked some common issues, I summarize them here:

1. The public of junit5 test method can be removed

You can search the keyword in the state package, and remove all public for them.

@Test
    public 
图片

2. Replace the isEqualTo(1) and isEqualTo(1L) with isOne()

图片 图片

3. Replace .size()).isEqualTo( with hasSize or hasSameSizeAs

The InternalPriorityQueue doesn't work with this suggesion, and Java list or collection can work.

图片

4. Check whether fail with catch can be replaced with assertThrownXxx

图片

Please update them in your free time, thanks~

@Jiabao-Sun
Copy link
Contributor Author

Thanks @1996fanrui for the hard review and sorry for the oversight.
Please help review it again when you have time.

@Jiabao-Sun
Copy link
Contributor Author

Thanks @1996fanrui for the continuous help.
Please help with it again when you have time.

@Jiabao-Sun
Copy link
Contributor Author

Test count verified:

master
passed: 1291, ignored: 98, total: 1389
updated:
passed: 1291, ignored: 98, total: 1383, disabled: 7

Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @Jiabao-Sun for the hard work and thanks @ferenc-csaky for the hard review!

LGTM

@1996fanrui 1996fanrui merged commit 554810a into apache:master Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants