Excludes for createTempFile forbidden method about checkstyle HOT 10 CLOSED

Please look at API for cleanup.

https://junit.org/junit5/docs/5.4.1/api/org/junit/jupiter/api/io/TempDir.html

Temporary Directory Deletion

When the end of the scope of a temporary directory is reached, i.e. when the test method or class has finished execution, JUnit will attempt to recursively delete all files and directories in the temporary directory and, finally, the temporary directory itself. In case deletion of a file or directory fails, an IOException will be thrown that will cause the test or test class to fail.

You can also see the same code if you browse the TempDir and its javadocs pulled in from maven.

• Clean Up

• By default, when the end of the scope of a temporary directory is reached,

• — when the test method or class has finished execution — JUnit will
• attempt to clean up the temporary directory by recursively deleting all files
• and directories in the temporary directory and, finally, the temporary directory
• itself. In case deletion of a file or directory fails, an {@link IOException}
• will be thrown that will cause the test or test class to fail.

It is better we use things already built for us, instead of trying to do our own workaround.

But, i am afraid if there is any similar annotation like https://github.com/tempdir as such for the createTempFile method.

It seems to me, all you need to do is create files in the temp directory you created from the annotation. As long as you specify the files go in such temp directory then things should work out. As mentioned above, the TempDir annotation will attempt to clean up any files in the directory recursively.

You can always verify this on your local. As the other issue mentioned, just change java.io.tmpdir to some other, empty directory to see if it remains empty after a test run. Just note as the docs specify, it is not guarantee things will get cleaned up. It depends on how the JVM exits, and if the OS allows the deletion (See normal file deletes in Java).

from checkstyle.

I have some confusion on this issue. I have highlighted the same here. #14414 (comment). Once, I have some clarity, we can start working.

from checkstyle.

@rnveach
cc @romani , @nrmancuso
Firstly, sorry to get involved.
Please correct me if my understanding is wrong....

Response to #14414 (comment) :

But, i am afraid if there is any similar annotation like @tempdir as such for the createTempFile method.

@relentless-pursuit , you are actually right. I don't think there is any alternative as such.
However, can't we solve the issue at hand keeping the createTempFile as it is ?

From #14220 (comment) :

Files.createTempDirectory does not obey the system property java.io.tmpdir and does not attempt to clean up the temporary directory on completion of the run.

The major issue that I see with createTempFie is the lack of clean-up property.

Since the files are already a part of temporaryFolder , could we not simply add an explicit deletion ?..
Won't a simple closing call to deleteOnExit() suffice ?? This can't be hard as only 2-3 instances of said usage are found per test

from checkstyle.

@rnveach cc @romani , @nrmancuso Firstly, sorry to get involved. Please correct me if my understanding is wrong....

Response to #14414 (comment) :

But, i am afraid if there is any similar annotation like @tempdir as such for the createTempFile method.

@relentless-pursuit , you are actually right. I don't think there is any alternative as such. However, can't we solve the issue at hand keeping the createTempFile as it is ?

From #14220 (comment) :

Files.createTempDirectory does not obey the system property java.io.tmpdir and does not attempt to clean up the temporary directory on completion of the run.

The major issue that I see with createTempFie is the lack of clean-up property.

Since the files are already a part of temporaryFolder , could we not simply add an explicit deletion ?.. Won't a simple closing call to deleteOnExit() suffice ?? This can't be hard as only 2-3 instances of said usage are found per test

No worries. Feel free to contribute to enrich the PR. As i had limited understanding in the beginning, and assumed we mandatorily need to create files, but I don't think that is the case. However, some test cases need actual files. And, i have used File.createFile instead of Files.createTempFile.

from checkstyle.

No worries. Feel free to contribute to enrich the PR. As i had limited understanding in the beginning, and assumed we mandatorily need to create files, but I don't think that is the case. However, some test cases need actual files. And, i have used File.createFile instead of Files.createTempFile.

Seems you have found a workaround ;)

As mentioned at #14414 (comment) ,
I'm still not sure if using the TempDir ensures that the temp files in the folder are always cleaned up though !!
Is this not of concern ?

from checkstyle.

#14414 (comment)

forbidding Files.createTempFile, is it needed?

If there is someway to bypass java.io.tmpdir and use the system's temp directory, then it needs to be banned IMO. This is what the other issue was about and it was approved and implemented. I haven't looked up Files.createTempFile but I assume one of the method signatures is to not provide a directory location and it will use something else instead.

Edit: In my implementation, createTempFile(String prefix, String suffix) calls createTempFile(prefix, suffix, null), null calls TempDirectory.location(), and the code around it uses "java.io.tmpdir". So it should be safe.

Based on this information, we may want to rethink banning Files.createTempFile as it seems safe.

from checkstyle.

#14414 (comment)

forbidding Files.createTempFile, is it needed?

If there is someway to bypass java.io.tmpdir and use the system's temp directory, then it needs to be banned IMO. This is what the other issue was about and it was approved and implemented. I haven't looked up Files.createTempFile but I assume one of the method signatures is to not provide a directory location and it will use something else instead.

Edit: In my implementation, createTempFile(String prefix, String suffix) calls createTempFile(prefix, suffix, null), null calls TempDirectory.location(), and the code around it uses "java.io.tmpdir". So it should be safe.

I have removed createTempFile. And, the test cases are wokring fine, except in some where i had to have a file. In my PR, i am just creating a path or, path to the file, and not the file itself, and eliminating the need of any tempFile. For most test cases, it worked fine. Meanwhile, i will analyse your review comments and understand better. Please let me know if the changes in the PR look good to you.

from checkstyle.

You can always verify this on your local. As the other issue mentioned, just change java.io.tmpdir to some other, empty directory to see if it remains empty after a test run. Just note as the docs specify, it is not guarantee things will get cleaned up. It depends on how the JVM exits, and if the OS allows the deletion (See normal file deletes in Java).

I did. So far, it seems to be cleaning up.

from checkstyle.

Won't a simple closing call to deleteOnExit() suffice ??

Well, it can be a solution. But, then it becomes a manual cleanup. And, it comes with it's own trade-off. As highlighted already, @TempDir, though does automate the process, but doesn't guarantee. So, maybe a good option rather doing it manually.

from checkstyle.

We missed removal of exclusions at https://github.com/Lmh-java/checkstyle/blob/16259a82bc779d4adbf329721cfa0eef33d79cd4/pom.xml#L1868

Edit: nevermind, contributor needed to rebase

from checkstyle.