HDDS-12807. Clean up TestKeyManagerImpl unit test#10640
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @echonesis for the patch. Overall LGTM, few minor improvements suggested.
| for (int j = 0; j < testCase.getNumberOfBucketsPerVolume(); j++) { | ||
| for (int k = 0; k < testCase.getNumberOfKeysPerBucket(); k++) { | ||
| String key = getTableKey(volumeNamePrefix, i, bucketNamePrefix, j, keyPrefix, k); | ||
| V value = valueClass == String.class ? (V) key : mock(valueClass); |
There was a problem hiding this comment.
This lets us avoid the need for SuppressWarnings("unchecked"):
| V value = valueClass == String.class ? (V) key : mock(valueClass); | |
| V value = valueClass == String.class ? valueClass.cast(key) : mock(valueClass); |
There was a problem hiding this comment.
Thanks for mentioning!
Update it in the next commit.
| throws IOException { | ||
| @ParameterizedTest(name = "{0}") | ||
| @MethodSource("getSuccessfulTableIteratorParameters") | ||
| public void testGetDeletedKeyEntries(TableIteratorTestCase testCase) throws IOException { |
There was a problem hiding this comment.
Class 'TableIteratorTestCase' is exposed outside its defined visibility scope
| public void testGetDeletedKeyEntries(TableIteratorTestCase testCase) throws IOException { | |
| void testGetDeletedKeyEntries(TableIteratorTestCase testCase) throws IOException { |
Also applies to other parameterized test methods.
Since JUnit 5, test methods do not need to be public. It's better to keep them package-private for simplicity/brevity regardless of this warning.
| String bucketNamePrefix = "bucket"; | ||
| String keyPrefix = "key"; | ||
| OzoneConfiguration configuration = new OzoneConfiguration(); | ||
| OMMetadataManager metadataManager = Mockito.mock(OMMetadataManager.class); |
There was a problem hiding this comment.
Mockito.mock is already imported
| OMMetadataManager metadataManager = Mockito.mock(OMMetadataManager.class); | |
| OMMetadataManager metadataManager = mock(OMMetadataManager.class); |
Also in few other places.
| return Long.parseLong(key.split("/")[3]); | ||
| } | ||
|
|
||
| private static final class TableIteratorTestCase { |
There was a problem hiding this comment.
nit: class can be renamed to TestCase since it's private
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @echonesis for updating the patch.
|
Thanks @adoroszlai for the review and merge. |
What changes were proposed in this pull request?
This PR cleans up
TestKeyManagerImplby replacing the long parameter lists in the parameterized table iterator tests with a smallTableIteratorTestCasebuilder.The change also separates successful and invalid table iterator cases, so the expected exception path is tested explicitly instead of being mixed into the common assertion flow.
Generated-by: OpenAI Codex (GPT-5)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12807
How was this patch tested?
Tested with:
GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/28430712787