-
Notifications
You must be signed in to change notification settings - Fork 903
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
test: migrate client module api tests to junit 5 #4377
test: migrate client module api tests to junit 5 #4377
Conversation
bcc59bb
to
da302e5
Compare
da302e5
to
e85971e
Compare
407fc29
to
f0aa9af
Compare
f0aa9af
to
859af11
Compare
...keeper-server/src/test/java/org/apache/bookkeeper/client/extension/TestContextExtension.java
Outdated
Show resolved
Hide resolved
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.
Use JUnit5's TestInfo instead of adding custom extension.
@lhotari Thanks for you remind, I remember this class too, and it has been used in bookkeeper/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java Lines 178 to 180 in 5f73147
|
...-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersOpenLedgerTest.java
Outdated
Show resolved
Hide resolved
8cb695d
to
2615c36
Compare
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.
minor nits: I think volatile in private volatile String testName;
might be useless
2615c36
to
84335b9
Compare
done :) |
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeperTestCase.java
Show resolved
Hide resolved
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeperTestCase.java
Show resolved
Hide resolved
@lhotari help review,please |
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.
LGTM
Good work @sherlock-lin |
I'll close and reopen to see if it passes with latest changes in master. |
Thanks for your contribution :) |
Descriptions of the changes in this PR:
Fix #4322
Main Issue: #4322