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-32858][tests][JUnit5 migration] Migrate flink-runtime/utils tests to JUnit5 #23199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @X-czh , thanks for the contribution.
I didn't finish the review, and just did a quick review. I found a few changes can be improved. These checks can be more specific by their types. Such as:
- The
AbstractIterableAssert
has a series of checks method for iterable:isEmpty
,hasSize
, etc.- Change the
assertThat(testInstance.size()).isEqualTo(0);
toassertThat(testInstance).isEmpty();
. - Change the
assertThat(testInstance.size()).isEqualTo(1);
toassertThat(testInstance).hasSize(1);
- Change the
- Using the
assertThat(pid)
instead ofassertThat(pid != -1).
- Change the
assertThat(pid != -1).withFailMessage("Cannot determine process ID").isTrue();
toassertThat(pid).withFailMessage("Cannot determine process ID").isNotEqualTo(-1);
- Change the
Would you mind finding relevant changes first and improving them? thanks~
flink-runtime/src/test/java/org/apache/flink/runtime/util/BlockingShutdownTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/BlockingShutdownTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/BoundedFIFOQueueTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/BoundedFIFOQueueTest.java
Outdated
Show resolved
Hide resolved
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JingGe , I'm not sure should these classes releted to junit4 be marked to Deprecated
. Would you mind helping check it? thanks a lot~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @X-czh , could you remove this commit, and squash all commit after address other comments?
Because most of junit4 classes are not marked to Deprecated
so far, if you want to do it, it's better to do it in a separate JIRA, and mark all classes togother.
WDYT?
Thanks for the comments! I've searched through the change list again and improved a few usages in a fixup commit. |
Hi @Jiabao-Sun , would you mind helping review this PR in your free time? thanks~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @X-czh for this contribution.
I left some comments.
flink-runtime/src/test/java/org/apache/flink/runtime/util/AddressResolutionTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/BoundedFIFOQueueTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/DualKeyLinkedMapTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/EnvironmentInformationTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/EnvironmentInformationTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/flink/runtime/util/config/memory/ManagedMemoryUtilsTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/flink/runtime/util/config/memory/ManagedMemoryUtilsTest.java
Outdated
Show resolved
Hide resolved
...me/src/test/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtilsTestBase.java
Outdated
Show resolved
Hide resolved
...me/src/test/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtilsTestBase.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/org/apache/flink/runtime/util/config/memory/TaskExecutorProcessSpecTest.java
Outdated
Show resolved
Hide resolved
Hi @Jiabao-Sun, thanks for your comments, I will address them today, |
Hi @Jiabao-Sun, I've addressed all the comments, take your time to review it again. Thanks~ |
Thanks @X-czh for the quick commits. |
Thanks @Jiabao-Sun, I've addressed the missing comments and applied an improvement to assert exception messages with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @X-czh for the contribution and @Jiabao-Sun for the review!
I left some minor comments, please take a look in your free time, thanks~
flink-runtime/src/test/java/org/apache/flink/runtime/util/BoundedFIFOQueueTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/BoundedFIFOQueueTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/RunnablesTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/SerializedThrowableTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/SerializedThrowableTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/SerializedThrowableTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/SerializedThrowableTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/flink/runtime/util/config/memory/ManagedMemoryUtilsTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/flink/runtime/util/config/memory/ManagedMemoryUtilsTest.java
Outdated
Show resolved
Hide resolved
...me/src/test/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtilsTestBase.java
Outdated
Show resolved
Hide resolved
Thanks for the review @1996fanrui! I've addressed all the comments. Take your time to review it, thanks~ |
flink-runtime/src/test/java/org/apache/flink/runtime/util/SerializedThrowableTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/SerializedThrowableTest.java
Outdated
Show resolved
Hide resolved
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @X-czh , could you remove this commit, and squash all commit after address other comments?
Because most of junit4 classes are not marked to Deprecated
so far, if you want to do it, it's better to do it in a separate JIRA, and mark all classes togother.
WDYT?
Thanks for the comments, I'll remove the deprecation commit and squash all commit after address other comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @X-czh for the hard work, and @Jiabao-Sun for the great review!
LGTM!
What is the purpose of the change
Migrate flink-runtime/utils tests to JUnit5.
Verifying this change
The change itself is for migrating UT.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation