-
Notifications
You must be signed in to change notification settings - Fork 1k
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
JarFileUtils.delete(File f) throw actual exception (instead of FileNotFound) when file cannot be deleted #2825 #2826
Conversation
Updated JarFileUtils delete method to provide more informative exception information (SonarQube RSPEC-4042)
…eption Fixed: GITHUB-2825: JarFileUtils.delete(File f) throw actual exception (instead of FileNotFound) when file cannot be deleted
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.
Is there any way to add a test that will enforce the fix?
if (!f.delete()) throw new FileNotFoundException("Failed to delete file: " + f); | ||
|
||
/** * Provide better logging for files temp that fail to be cleaned up. */ | ||
if (!Files.deleteIfExists(f.toPath())) { |
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 think there is no need of comments here because deleteIfExists is definitively better.
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.
Removed the comment
|
||
/** * Provide better logging for files temp that fail to be cleaned up. */ | ||
if (!Files.deleteIfExists(f.toPath())) { | ||
Utils.log("TestNG", 2, "Failed to delete non-existent temp file: " + f.getPath()); |
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.
The behavior is changed and maybe some users prefere the current one.
Please make the behavior change configurable.
By default the exception looks better because we expect the deletion.
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.
Removed the logging option, I agree that it should throw an exception instead.
Removed logging
I was going to write a unit test for the delete method but it is private, I did not think it would be a good idea to make it public. Some of the other tests should already fail if this method throws an exception, but it seems like this is a rare circumstance that I've so far been the only one to experience on my specific platform and application. |
Fixes #2825 .
Changed JarFileUtils.delete(File f) throw actual exception (instead of FileNotFound) when file cannot be deleted, as recommended by SonarSource (https://rules.sonarsource.com/java/tag/error-handling/RSPEC-4042).
If the file is not found, a log message is generated.
Files.deleteIfExists() also throws:
Did you remember to?
CHANGES.txt
./gradlew autostyleApply