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.

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

Merged
merged 6 commits into from Nov 7, 2022

Conversation

speedythesnail
Copy link
Contributor

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:

DirectoryNotEmptyException - if the file is a directory and could not otherwise be deleted because the directory is not empty (optional specific exception)

IOException - if an I/O error occurs

SecurityException - In the case of the default provider, and a security manager is installed, the SecurityManager.checkDelete(String) method is invoked to check delete access to the file.

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt
  • Auto applied styling via ./gradlew autostyleApply

speedythesnail and others added 5 commits November 5, 2022 22:17
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
Copy link
Member

@juherr juherr left a 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?

.gitignore Show resolved Hide resolved
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


/** * 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());
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.

Removed logging
@speedythesnail
Copy link
Contributor Author

Is there any way to add a test that will enforce the fix?

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.

@speedythesnail speedythesnail requested review from juherr and removed request for krmahadevan November 6, 2022 16:09
@krmahadevan krmahadevan merged commit ae496a3 into testng-team:master Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Programically Loading TestNG Suite from JAR File Fails to Delete Temporary Copy of Suite File
3 participants