Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JarFileUtils.delete(File f) throw actual exception (instead of FileNotFound) when file cannot be deleted #2825 #2826

Merged
merged 6 commits into from Nov 7, 2022
Merged

JarFileUtils.delete(File f) throw actual exception (instead of FileNotFound) when file cannot be deleted #2825 #2826

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b79d5d7
Update JarFileUtils.java
speedythesnail Nov 6, 2022
b6ec53b
Update CHANGES.txt
speedythesnail Nov 6, 2022
0af1bac
Added **/bin/ to .gitignore
speedythesnail Nov 6, 2022
42fcdfe
Added logging to JarUtils.delete()
speedythesnail Nov 6, 2022
e1b9895
Merge pull request #1 from speedythesnail/Improve_JarUtils_Delete_Exc…
speedythesnail Nov 6, 2022
890ecd9
Update JarFileUtils.java
speedythesnail Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -26,3 +26,4 @@ z_build
.DS_Store
outputDir/
**/Version.java
**/bin/
juherr marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions CHANGES.txt
View file
Open in desktop
@@ -1,4 +1,5 @@
Current
Fixed: GITHUB-2825: JarFileUtils.delete(File f) throw actual exception (instead of FileNotFound) when file cannot be deleted (Steven Jubb)
Fixed: GITHUB2818: Add configuration key for callback discrepancy behavior (Krishnan Mahadevan)
Fixed: GITHUB-2819: Ability to retry a data provider in case of failures (Krishnan Mahadevan)
Fixed: GITHUB-2308: StringIndexOutOfBoundsException in findClassesInPackage - Surefire/Maven - JDK 11 fails (Krishnan Mahadevan)
Expand Down
7 changes: 5 additions & 2 deletions testng-core/src/main/java/org/testng/JarFileUtils.java
View file
Open in desktop
@@ -1,7 +1,6 @@
package org.testng;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
Expand Down Expand Up @@ -112,7 +111,11 @@ private void delete(File f) throws IOException {
if (f.isDirectory()) {
for (File c : Objects.requireNonNull(f.listFiles())) delete(c);
}
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())) {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment

Utils.log("TestNG", 2, "Failed to delete non-existent temp file: " + f.getPath());
Copy link
Member

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.

Copy link
Contributor Author

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.

}
}

private boolean matchesXmlPathInJar(JarEntry je) {
Expand Down