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-32837][JUnit5 Migration] Migrate the client and clusterframework packages of flink-runtime module to junit5 #23241

Merged
merged 2 commits into from Aug 23, 2023
Merged

[FLINK-32837][JUnit5 Migration] Migrate the client and clusterframework packages of flink-runtime module to junit5 #23241

merged 2 commits into from Aug 23, 2023

Conversation

1996fanrui
Copy link
Member

[FLINK-32837][JUnit5 Migration] Migrate the client and clusterframework packages of flink-runtime module to junit5

@1996fanrui
[FLINK-32837][JUnit5 Migration] Migrate the client and clusterframewo…
08fe96c 
…rk packages of flink-runtime module to junit5
@1996fanrui 1996fanrui force-pushed the 32837/client-cluster-framework-junit5 branch from 7225095 to 08fe96c Compare August 18, 2023 16:31
@1996fanrui
Copy link
Member Author

cc @Jiabao-Sun

@1996fanrui 1996fanrui marked this pull request as ready for review August 18, 2023 16:34
@flinkbot
Copy link
Collaborator

flinkbot commented Aug 18, 2023 •
edited

CI report:

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

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 @1996fanrui for this great work.
I left some comments for improvements.

flink-runtime/src/test/java/org/apache/flink/runtime/jobmanager/JobManagerProcessUtilsTest.java

/** Tests for {@link JobManagerProcessUtils}. */
public class JobManagerProcessUtilsTest extends ProcessMemoryUtilsTestBase<JobManagerProcessSpec> {
class JobManagerProcessUtilsTest extends ProcessMemoryUtilsTestBase<JobManagerProcessSpec> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test class seems in module jobmanager.
Is there any dependence?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the class in jobmanager module.

I migrated it because JobManagerProcessUtilsTest and TaskExecutorProcessUtilsTest extends ProcessMemoryUtilsTestBase.

I want to migrate these 3 classes to junit5 together, however ProcessMemoryUtilsTestBase is migrated at #23199, so I updated JobManagerProcessUtilsTest and TaskExecutorProcessUtilsTest at this PR.

Do you think it is ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to me.

flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java Outdated
Comment on lines 642 to 645
assertThat(res).hasSize(1);
Map.Entry<String, String> entry = res.entrySet().iterator().next();
Assert.assertEquals("LD_LIBRARY_PATH", entry.getKey());
Assert.assertEquals("/usr/lib/native", entry.getValue());
assertThat(entry.getKey()).isEqualTo("LD_LIBRARY_PATH");
assertThat(entry.getValue()).isEqualTo("/usr/lib/native");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it can be simplified as

assertThat(res).hasSize(1).containsEntry("LD_LIBRARY_PATH", "/usr/lib/native")

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, it's very concise! updated

flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java Outdated
Comment on lines 669 to 670
assertThat(flinkConfig.get(listStringConfigOption))
.containsAnyOf(list.toArray(new String[0]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThat(flinkConfig.get(listStringConfigOption))
.containsAnyOf(list.toArray(new String[0]));
assertThat(flinkConfig.get(listStringConfigOption))
.containsExactlyInAnyOrderElementsOf(list);

flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java Outdated
Comment on lines 680 to 681
assertThat(flinkConfig.get(listDurationConfigOption))
.containsAnyOf(durationList.toArray(new Duration[0]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThat(flinkConfig.get(listDurationConfigOption))
.containsAnyOf(durationList.toArray(new Duration[0]));
assertThat(flinkConfig.get(listDurationConfigOption))
.containsExactlyInAnyOrderElementsOf(durationList);

flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java Outdated
Comment on lines 703 to 706
assertThat(loadedFlinkConfig.get(listStringConfigOption))
.contains(list.toArray((new String[0])));
assertThat(loadedFlinkConfig.get(listDurationConfigOption))
.contains(durationList.toArray((new Duration[0])));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThat(loadedFlinkConfig.get(listStringConfigOption))
.contains(list.toArray((new String[0])));
assertThat(loadedFlinkConfig.get(listDurationConfigOption))
.contains(durationList.toArray((new Duration[0])));
assertThat(loadedFlinkConfig.get(listStringConfigOption))
.containsExactlyInAnyOrderElementsOf(list);
assertThat(loadedFlinkConfig.get(listDurationConfigOption))
.containsExactlyInAnyOrderElementsOf(durationList);

1996fanrui
1996fanrui commented Aug 20, 2023
View reviewed changes
Copy link
Member Author

@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 's review, I have updated all comments, please help check them in your free time, thanks~

flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java Outdated
Comment on lines 642 to 645
assertThat(res).hasSize(1);
Map.Entry<String, String> entry = res.entrySet().iterator().next();
Assert.assertEquals("LD_LIBRARY_PATH", entry.getKey());
Assert.assertEquals("/usr/lib/native", entry.getValue());
assertThat(entry.getKey()).isEqualTo("LD_LIBRARY_PATH");
assertThat(entry.getValue()).isEqualTo("/usr/lib/native");
Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, it's very concise! updated

flink-runtime/src/test/java/org/apache/flink/runtime/jobmanager/JobManagerProcessUtilsTest.java

/** Tests for {@link JobManagerProcessUtils}. */
public class JobManagerProcessUtilsTest extends ProcessMemoryUtilsTestBase<JobManagerProcessSpec> {
class JobManagerProcessUtilsTest extends ProcessMemoryUtilsTestBase<JobManagerProcessSpec> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the class in jobmanager module.

I migrated it because JobManagerProcessUtilsTest and TaskExecutorProcessUtilsTest extends ProcessMemoryUtilsTestBase.

I want to migrate these 3 classes to junit5 together, however ProcessMemoryUtilsTestBase is migrated at #23199, so I updated JobManagerProcessUtilsTest and TaskExecutorProcessUtilsTest at this PR.

Do you think it is ok?

Jiabao-Sun
Jiabao-Sun approved these changes 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 @1996fanrui for the quick update.
LGTM.

@1996fanrui 1996fanrui merged commit 64c725f into apache:master Aug 23, 2023
@1996fanrui 1996fanrui deleted the 32837/client-cluster-framework-junit5 branch August 23, 2023 13:51
wuqqq pushed a commit to wuqqq/flink that referenced this pull request Aug 23, 2023
@1996fanrui @wuqqq
[FLINK-32837][JUnit5 Migration] Migrate the client and clusterframewo…
20a2a3c 
…rk packages of flink-runtime module to junit5 (apache#23241)
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 approved these changes

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