-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-42101: [Java] Create File for Output Validation in FileRoundtrip #42115
Conversation
|
() -> { | ||
new FileRoundtrip(System.err).run(args); | ||
}); | ||
assertEquals("file too small: 0", exception.getMessage()); |
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.
If this PR is merged, changes will be needed to make it compatible with JUnit 5.
When using JUnit 5's @TempDir
annotation, it was found that the logic works with non-existent files, causing IllegalArgumentException
with the message input file not found
.
This seems to be due to differences in behavior between JUnit 4's @TemporaryFolder
and JUnit 5's @TempDir
annotations.
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.
Overall LGTM! Let's check the CIs.
"Failed to create parent directory: " + parentDir.getAbsolutePath()); | ||
} | ||
} | ||
if (!f.exists()) { |
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.
Hmm, why does the output file need to exist?
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 JUnit 4, there was an annotation called @TemporaryFolder
, but in JUnit 5, it's changed to @TempDir
. This means that the way temporary files are created has changed too.
So, just changing the annotation in the old test code wasn't enough to make the tests pass. I had to add some code to create the output
file using createNewFile()
to make the tests work.
arrow/java/tools/src/test/java/org/apache/arrow/tools/TestFileRoundtrip.java
Lines 55 to 59 in 298786a
if (!testOutFile.exists()) { | |
if (!testOutFile.createNewFile()) { | |
throw new IOException("Failed to create file: " + testOutFile); | |
} | |
} |
While working with the FileRoundtrip
class, I thought of two ways to handle this (you can check the link for details). The input
is always needed, but the output
path or file might not always exist (especially if it's different from the input
path).
So, I added some code to create the output
path or file if they don't exist, before the rest of the logic runs.
If you think there are better ways to do this or if something's unclear, I'd appreciate your advice.
Here’s the link explaining why this change was needed.
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.
Er, but again...I can see why the output directory needs to exist, but why does the output file have to exist?
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.
Aha! You are right. We don't need to create the file in validateFile
. This is because the run
method executes try (FileOutputStream fileOutputStream = new FileOutputStream(outFile);
.
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 removed it.
() -> { | ||
new FileRoundtrip(System.err).run(args); | ||
}); | ||
assertEquals("file too small: 0", exception.getMessage()); |
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.
What is actually being tested here? A 0-length input file?
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.
No.
In JUnit 5, I want to check for IllegalArgumentException
and input file not found
errors.
The code I wrote still works with JUnit 4 logic, so the tests don't write the input (writeInput(testInFile, allocator)
). Because the file exists, I can see these exceptions happening.
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 02f461f. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 16 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Enhance the logic to ensure that the output directory and file are created if they do not exist. While the input directory and file are mandatory, the output directory and file might not exist.
What changes are included in this PR?
Are these changes tested?
Yes. Additional unit tests have been added.
Are there any user-facing changes?
Maybe. Yes.