-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Replace if-return with Assume API from Junit in DecimalParquetInputTest
#16436
Conversation
DecimalParquetInputTest
DecimalParquetInputTest
DecimalParquetInputTest
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 for the changes, @Codegass . I have left some comments.
if (parserType.equals(ParquetExtensionsModule.PARQUET_AVRO_INPUT_PARSER_TYPE)) { | ||
return; | ||
} | ||
assumeFalse(parserType.equals(ParquetExtensionsModule.PARQUET_AVRO_INPUT_PARSER_TYPE)); |
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.
Druid code doesn't use the import static style.
assumeFalse(parserType.equals(ParquetExtensionsModule.PARQUET_AVRO_INPUT_PARSER_TYPE)); | |
Assume.assumeFalse(parserType.equals(ParquetExtensionsModule.PARQUET_AVRO_INPUT_PARSER_TYPE)); |
"example/decimals/dec_in_i64.json", | ||
parserType, | ||
true | ||
"example/decimals/dec_in_i64.json", |
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.
These changes are not needed as this code is already formatted correctly according to the formatting style used by this project. Same comment for the other formatting changes.
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.
thank you for the suggestions! I just updated the pr based on them.
...et-extensions/src/test/java/org/apache/druid/data/input/parquet/DecimalParquetInputTest.java
Outdated
Show resolved
Hide resolved
...et-extensions/src/test/java/org/apache/druid/data/input/parquet/DecimalParquetInputTest.java
Outdated
Show resolved
Hide resolved
...et-extensions/src/test/java/org/apache/druid/data/input/parquet/DecimalParquetInputTest.java
Outdated
Show resolved
Hide resolved
...et-extensions/src/test/java/org/apache/druid/data/input/parquet/DecimalParquetInputTest.java
Outdated
Show resolved
Hide resolved
Thanks a lot for your first contribution to Druid, @Codegass ! |
Fixes #12926.
Description
This PR improves the test cases in
DecimalParquetInputTest
by replacing the if-return blocks that check for unsupported parser types withassumeFalse
-- Assume API from Junit.Replaced if-return blocks with assumeFalse
The existing test cases used if-return blocks like this to skip tests for unsupported parser types:
This made the test logic implicit. It's preferable to make unsupported preconditions explicit using JUnit's
assume
methods. The assume function, introduced after JUnit 4, is designed to check for test pre-condition. As mentioned in the document:I replaced the if-return blocks with equivalent
assumeFalse
assertions:A failed assumption will cause the test to be skipped, while still allowing the test to run for supported parser types. This makes the preconditions clearer without changing the test behavior.
This refactoring resolves the silent quit issue by using the Assume API to provide explicit output when tests are skipped.
I applied this change to all three test cases in
DecimalParquetInputTest
:testReadParquetDecimalFixedLen
testReadParquetDecimali32
testReadParquetDecimali64
This is a straightforward refactoring that improves test readability without altering functionality. No other code changes were made.
This PR has: