[BEAM-2682] Deletes AvroIOTransformTest#3999
Conversation
|
retest this please |
|
Changes Unknown when pulling b59a742 on jkff:rm-avroio-transform into ** on apache:master**. |
|
Ping? |
|
@jkff I started the review, should be done by the end of the day. |
echauchot
left a comment
There was a problem hiding this comment.
Thanks for simplifying all these tests Eugene!
I only have a couple of suggestions, a question and tiny minor changes.
| @Test | ||
| @Category(NeedsRunner.class) | ||
| public void testAvroIOWriteAndReadAndParseASingleFile() throws Throwable { | ||
| public void testAvroIOWriteAndReadJavaClass() throws Throwable { |
There was a problem hiding this comment.
Can you add a test for AvroIO.writeCustomType() even if it was not tested before and even if there is a test for its GenericRecord specialization?
There was a problem hiding this comment.
I wonder if that makes any sense to also test AvroIO.write(AvroGeneratedUser.class) and AvroIO.read(AvroGeneratedUser.class). The only difference with the existing test is that the input class is not a user provided class but an avro generated one. As behind the scene same SpecificDatumWriter<Class> will be called, the code might not behave differently. WDYT?
There was a problem hiding this comment.
Done, added coverage for these, thanks for pointing it out.
| public void testAvroIOWriteAndReadJavaClass() throws Throwable { | ||
| List<GenericClass> values = | ||
| ImmutableList.of(new GenericClass(3, "hi"), new GenericClass(5, "bar")); | ||
| File outputFile = tmpFolder.newFile("output.avro"); |
There was a problem hiding this comment.
Can you define a constant for the file name?
There was a problem hiding this comment.
I don't think it's worth it. The value of this string literal doesn't matter and it can be changed without breaking the code.
There was a problem hiding this comment.
yes, since all "output.avro" in this class are not related to each other, I agree. fair enough.
|
|
||
| @Test | ||
| @Category(NeedsRunner.class) | ||
| public void testAvroIOWriteAndReadAndParseJavaClassSingleFile() throws Throwable { |
There was a problem hiding this comment.
The name of the method is very precise but still lacks some cases that it tests. IMHO I would rename to something less precise like testAvroIOWriteAndReadAllMeansJavaClass.
There was a problem hiding this comment.
Renamed this one and renamed other tests to be more consistent.
| AvroIO.writeGenericRecords(SCHEMA) | ||
| .to(writePipeline.newProvider(outputFile.getAbsolutePath())) | ||
| .withoutSharding()); | ||
| writePipeline.run().waitUntilFinish(); |
There was a problem hiding this comment.
Why is there sometimes a blocking call to writePipeline.run().waitUntilFinish(); and sometimes a non blocking call to writePipeline.run() for example l975 of AvroIOTest? In l975 for example, files written by the writePipeline are read just after the non blocking call to writePipeline.run()
There was a problem hiding this comment.
All waitUntilFinish() calls were unnecessary, I removed them.
Instead, merges the little test coverage it provided into AvroIOTest.
b59a742 to
6b43f6a
Compare
Instead, merges the little test coverage it provided into AvroIOTest.
This supersedes #3948 - merging these two test classes is long overdue and AvroIOTransformTest was quite bad code:
R: @echauchot