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-32851][runtime][JUnit5 Migration] The rest package of flink-runtime module #23242

Closed
wants to merge 2 commits into from
Closed

[FLINK-32851][runtime][JUnit5 Migration] The rest package of flink-runtime module #23242

wants to merge 2 commits into from

Conversation

wangzzu
Copy link
Contributor

@wangzzu wangzzu commented Aug 19, 2023

What is the purpose of the change

Migrate rest package related tests under flink-runtime module to junit5

Brief change log

Migrate rest package related tests under flink-runtime module to junit5

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

@flinkbot
Copy link
Collaborator

flinkbot commented Aug 19, 2023 •
edited

CI report:

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

1996fanrui
1996fanrui reviewed Aug 20, 2023
View reviewed changes
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 @wangzzu , thanks for the contribution, the CI failed due to checkstyle[1], please run mvn spotless:apply to fix it.

And @Jiabao-Sun , would you mind helping review this PR first? thanks~

[1] https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=52438&view=logs&j=52b61abe-a3cc-5bde-cc35-1bbe89bb7df5&t=54421a62-0c80-5aad-3319-094ff69180bb&l=11204

Jiabao-Sun
Jiabao-Sun reviewed Aug 20, 2023
View reviewed changes
Copy link
Contributor

@Jiabao-Sun Jiabao-Sun left a comment

Choose a reason for hiding this comment

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

Thanks @wangzzu for this contribution.
I left some comments about try { fail("") } catch exception assertion.
I prefer to simplify to assertThatThrownBy().

flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java Outdated Show resolved Hide resolved
@wangzzu
Copy link
Contributor Author

wangzzu commented Aug 21, 2023

@Jiabao-Sun thx for your reviewing, i have fixed it

Jiabao-Sun
Jiabao-Sun reviewed Aug 21, 2023
View reviewed changes
Copy link
Contributor

@Jiabao-Sun Jiabao-Sun left a comment

Choose a reason for hiding this comment

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

Thanks @wangzzu for the quick fix.
The branch seems has some conflicts.
Could you help rebase master?

flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestExternalHandlersITCase.java Outdated Show resolved Hide resolved
...runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointConfigurationTest.java Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java Outdated Show resolved Hide resolved
Jiabao-Sun
Jiabao-Sun reviewed Aug 21, 2023
View reviewed changes
Copy link
Contributor

@Jiabao-Sun Jiabao-Sun left a comment

Choose a reason for hiding this comment

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

Thanks @wangzzu for the quick update.
I left some comments. Please take a look when you have time.
This is a big PR, I might need the next round to complete it.

...me-web/src/test/java/org/apache/flink/runtime/webmonitor/handlers/JarRunRequestBodyTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientMultipartTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java Outdated Show resolved Hide resolved
.../test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandlerTest.java Outdated Show resolved Hide resolved
.../test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandlerTest.java Outdated Show resolved Hide resolved
...rc/test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandlerTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/job/JobSubmitHandlerTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/job/JobSubmitHandlerTest.java Outdated Show resolved Hide resolved
@Jiabao-Sun
Copy link
Contributor

By the way, the CI looks failed.
Please take a look as well.

@wangzzu
Copy link
Contributor Author

wangzzu commented Aug 22, 2023

@Jiabao-Sun thanks for your hard views, i have fixed these, if you have time, can you help review this again

@wangzzu
Copy link
Contributor Author

wangzzu commented Sep 4, 2023

@1996fanrui @Jiabao-Sun if you have time, can you help me reveiw this again

@huwh huwh self-requested a review September 15, 2023 07:10
@wangzzu wangzzu force-pushed the FLINK-32851 branch 3 times, most recently from df6618a to 7abdf8b Compare September 20, 2023 03:30
huwh
huwh reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@huwh huwh left a comment

Choose a reason for hiding this comment

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

Thanks @wangzzu for your hard work. I left some comments, please take a look.

...e/src/test/java/org/apache/flink/runtime/rest/handler/async/CompletedOperationCacheTest.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestExternalHandlersITCase.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java Outdated Show resolved Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java Outdated Show resolved Hide resolved
...ava/org/apache/flink/runtime/rest/handler/job/metrics/AggregatingMetricsHandlerTestBase.java Outdated Show resolved Hide resolved
...ava/org/apache/flink/runtime/rest/handler/job/metrics/AggregatingMetricsHandlerTestBase.java Outdated Show resolved Hide resolved
...ava/org/apache/flink/runtime/rest/handler/job/metrics/AggregatingMetricsHandlerTestBase.java Outdated Show resolved Hide resolved
...c/test/java/org/apache/flink/runtime/rest/handler/legacy/DefaultExecutionGraphCacheTest.java Outdated Show resolved Hide resolved
...ime/src/test/java/org/apache/flink/runtime/rest/messages/json/JobResultDeserializerTest.java Outdated Show resolved Hide resolved
...ime/src/test/java/org/apache/flink/runtime/rest/messages/json/JobResultDeserializerTest.java Outdated Show resolved Hide resolved
@wangzzu
Copy link
Contributor Author

wangzzu commented Sep 20, 2023

@huwh thanks for your review, i have fixed these

@wangzzu
Copy link
Contributor Author

wangzzu commented Sep 20, 2023

@Jiabao-Sun @1996fanrui do you have time to review it again?

huwh
huwh approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@huwh huwh 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 quick update. LGTM.

@huwh
Copy link
Contributor

huwh commented Sep 22, 2023

There hasn't been a response in a while. Merging now.

@huwh huwh closed this in 4817db9 Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jiabao-Sun Jiabao-Sun Jiabao-Sun left review comments

@1996fanrui 1996fanrui 1996fanrui left review comments

@huwh huwh huwh approved these changes

Assignees
No one assigned
Labels
component=Tests
Projects
None yet
Milestone
No milestone
5 participants
@wangzzu @flinkbot @Jiabao-Sun @huwh @1996fanrui