-
Notifications
You must be signed in to change notification settings - Fork 892
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
base: master
Are you sure you want to change the base?
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
/** | ||
* Unit Test Context Information. | ||
*/ | ||
public class TestContextExtension implements BeforeEachCallback { |
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.
I think we can put this common class into testtools
, WDYT?
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.
I think we can put this common class into
testtools
, WDYT?
Of course, I would also like this class to become more generalized, how do I add it to testtools 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.
This class isn't needed. Junit5 already contains TestInfo for this purpose.
Please check https://junit.org/junit5/docs/current/user-guide/#writing-tests-dependency-injection for the example.
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
|
public static Collection<Object[]> data() { | ||
return Arrays.asList(new Object[][]{ | ||
{true}, | ||
{false} | ||
}); | ||
} | ||
|
||
@Test | ||
public void testOpenLedger() throws Exception { | ||
@MethodSource("data") |
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.
In junit5, it's better to use @ValueSource(booleans = {true, false})
for cases where there's a single boolean parameter.
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.
@lhotari Thanks,It's a better choice.
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 :) |
@@ -264,6 +267,7 @@ private DigestManager getDigestType(long ledgerId) throws GeneralSecurityExcepti | |||
UnpooledByteBufAllocator.DEFAULT, false); | |||
} | |||
|
|||
@AfterEach | |||
@After |
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.
how about add some todo here: // TODO: delete @Before when only run with junit5
@@ -141,6 +143,7 @@ public MockEntry(byte[] payload, long lastAddConfirmed) { | |||
|
|||
} | |||
|
|||
@BeforeEach | |||
@Before |
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.
how about add some todo here: // TODO: delete @Before when only run with junit5
Descriptions of the changes in this PR:
Fix #4322
Main Issue: #4322