HADOOP-17868. Add more tests for BuiltInGzipCompressor #3336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @sunchao I added some more compatibility tests between So far, I don't see any incompatibility. |
steveloughran
requested changes
Aug 26, 2021
There was a problem hiding this comment.
Looks good, new tests which work reliably are always welcome.
We're adopting AssertJ in new code as part of the move to JUnit 5...it'd be good to use it here. And include error text in each assertion failure.
Imagine you are trying to debug a test for earlier from nothing but the error message. What would you provide in it?
| LOG.info("output: " + outputOff); | ||
| LOG.info("dflchk: " + dflchk.length); | ||
|
|
||
| assertEquals(outputOff, dflchk.length); |
There was a problem hiding this comment.
nit: add an error string in the assert
| } | ||
|
|
||
| DataOutputBuffer dflbuf = new DataOutputBuffer(); | ||
| GZIPOutputStream gzout = new GZIPOutputStream(dflbuf); |
There was a problem hiding this comment.
nit: use try-with-resources so the streams are automatically/always closed
| Random r = new Random(); | ||
| long seed = r.nextLong(); | ||
| r.setSeed(seed); | ||
| LOG.info("seed: " + seed); |
There was a problem hiding this comment.
nit, use LOG.info("seed {}", seed) here and the same elsewhere
| gzbuf.reset(output, outputLen); | ||
|
|
||
| Decompressor decom = codec.createDecompressor(); | ||
| assertNotNull(decom); |
There was a problem hiding this comment.
+exception text here and below. Ideally, use AssertJ assertions for new tests
|
Thanks @steveloughran. I will address the comments. |
|
Addressed the comments. Use AssertJ assertions and add exception message. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran @sunchao Please take another look. Thanks! |
sunchao
reviewed
Aug 30, 2021
| byte[] b = new byte[inputSize]; | ||
| r.nextBytes(b); | ||
|
|
||
| compressor.setInput(b,0, b.length); |
| assertThat(decom).as("decompressor should not be null").isNotNull(); | ||
| assertThat(decom).withFailMessage("should be BuiltInGzipDecompressor") | ||
| .isInstanceOf(BuiltInGzipDecompressor.class); | ||
| try (InputStream gzin = codec.createInputStream(gzbuf, decom);) { |
| assertThat(decom).withFailMessage("should be BuiltInGzipDecompressor") | ||
| .isInstanceOf(BuiltInGzipDecompressor.class); | ||
| try (InputStream gzin = codec.createInputStream(gzbuf, decom);) { | ||
|
|
| .isInstanceOf(BuiltInGzipDecompressor.class); | ||
| try (InputStream gzin = codec.createInputStream(gzbuf, decom);) { | ||
|
|
||
| DataOutputBuffer dflbuf = new DataOutputBuffer(); |
There was a problem hiding this comment.
nit: put this into the try block too? since it is also a resource that should be closed with a finally block
| Configuration conf = new Configuration(); | ||
| CompressionCodec codec = ReflectionUtils.newInstance(GzipCodec.class, conf); | ||
|
|
||
| for (int i = 0; i < 100; i++){ |
There was a problem hiding this comment.
can we share this code across the 3 tests?
There was a problem hiding this comment.
they are somehow different a bit. seems not too much to share between them.
| } | ||
|
|
||
| try (DataOutputBuffer dflbuf = new DataOutputBuffer(); | ||
| GZIPOutputStream gzout = new GZIPOutputStream(dflbuf);) { |
|
🎊 +1 overall
This message was automatically generated. |
sunchao
approved these changes
Aug 31, 2021
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/compress/TestCodec.java
Outdated
Show resolved
Hide resolved
|
@steveloughran Is this good for you now? Please take another look. Thanks. |
|
kindly ping @steveloughran |
|
As this is test-only change, as I think I should address all comments. If @steveloughran is too busy to response, could we just move forward? @sunchao |
|
LGTM. @viirya could you address the check style issues - I'll merge this after |
|
Thanks @sunchao ! Updated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@viirya there are still two more tiny style issues |
|
Fixed. Weird, I didn't see the two in last report. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
No style issue anymore. |
|
Merged to trunk. Thanks @viirya and @steveloughran ! |
|
Thank you @sunchao @steveloughran ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We added BuiltInGzipCompressor recently. It is better to add more compatibility tests for the compressor.