Strict verification of resource deletion in TemporaryFolder rule #1044

npathai · 2014-12-10T07:35:27Z

PR that adds support of strict verification of deleted resources when using TemporaryFolder rule #1001

kcooney · 2014-12-10T17:21:01Z

src/main/java/org/junit/rules/ExternalResource.java

@@ -65,7 +65,7 @@ protected void before() throws Throwable {
    /**
     * Override to tear down your specific external resource.
     */
-    protected void after() {
+    protected void after() throws Throwable {


We can't change the signature of published methods

npathai · 2014-12-15T06:15:33Z

@kcooney I have updated the code please review.
Some points:

I have named the parameter assuredDeletion. Looked promising to me.
I have kept the Builder because firstly the option is optional. If we don't go with the builder then will have to create overloads of constructors which accept parentFolder with parameter and other that accepted just this parameter. That looked messy. IMHO Builder is better.
Have not checked whether the parameter is set after apply is called, because ExpectedException does not check for it. And I think that would complicate the code. I am unable to understand the cons if we allow the flag to be set after apply.
Help me in deciding which exception to throw when cleanup fails? I have used IllegalStateException which I know is incorrect. But we need some RuntimeException? Please suggest. We may create a custom exception, but I don't know if that's a good idea.

kcooney · 2014-12-15T16:15:36Z

@marcphilipp you were the one who suggested we shouldn't add a builder. Could you respond to that part?

kcooney · 2014-12-15T16:17:10Z

src/main/java/org/junit/rules/TemporaryFolder.java

+        private boolean assureDeletion;
+
+        /**
+         * 


Could you add something useful here? Note you don't need to add Javadoc for params if they are abious based on the method Javadoc. Somewhere we should document want the parent folder is used for

@kcooney Done.

npathai · 2014-12-16T08:23:40Z

@kcooney Incorporated all the suggested changes. Only the mutability part is remaining to be discussed and finalized. Will take care of style guide in future.
Mental note: Follow google style guide

marcphilipp · 2014-12-16T11:52:17Z

src/main/java/org/junit/rules/TemporaryFolder.java

-            recursiveDelete(folder);
+        if (!tryDelete()) {
+            if (isDeletionAssured()) {
+                throw new IllegalStateException("Unable to clean up temporary folder " + folder);


IOException?

Edit: Not a good idea. We need an unchecked exception.

The only other things that comes to mind is to use an AssertionError or even assertTrue("Unable to clean up temporary folder " + folder, isDeletionAssured()). What do you think?

On the other hand, I could live with IllegalStateException, too.

IllegalStateException doesn't sound right to me because it "signals that a method has been invoked at an illegal or inappropriate time"

I think we should either throw AssertionError or RuntimeException. In either case, we could introduce a custom exception in the future.

Let's go with AssertionError. I have a sight preference for using fail() vs. assertTrue() here

marcphilipp · 2014-12-16T12:14:37Z

Builder

I am not sure how many people use the TemporaryFolder(File) constructor. If you don't, it's a bit wordy to use now:

@Rule public TemporaryFolder temporaryFolder = TemporaryFolder.builder().assureDeletion().build();

compared to a static factory method

@Rule public TemporaryFolder temporaryFolder = TemporaryFolder.assureDeletion();

We could do both but that might be confusing, too. I'd say let's keep the builder and drop the static factory method.

Mutability

Sorry for the confusion. @kcooney has convinced me in the discussion in #1001 that rules should rather not allow to have their behavior changed after they are constructed (ExpectedException being a notable exception to this rule). Thus, please make assureDeletion final and remove setter (and getter).

npathai · 2014-12-16T12:56:46Z

@marcphilipp I have made assureDeletion final and removed getter/setter. For now I have left IllegalStateException as it is. Let's see what @kcooney has to say on this.

Behavior

TemporaryFolder.builder().assureDeletion().build() will be the only way to opt for verification and there would be no constructor
assureDeletion will be applicable for all test cases and cannot be opted for a single test case

kcooney · 2014-12-16T19:39:48Z

src/main/java/org/junit/rules/TemporaryFolder.java

@@ -29,15 +29,86 @@
 public class TemporaryFolder extends ExternalResource {
    private final File parentFolder;
    private File folder;
-
+    private final boolean assureDeletion;


style-nit: let's but final fields before mutable fields. Thanks

npathai · 2014-12-17T05:10:43Z

@kcooney I have used fail() to report failure.
Some questions:

Should we deprecate the constructor with parentFolder in favor of Builder?
Should I put @since annotation? But in which milestone are we going to ship this feature?

kcooney · 2014-12-19T08:05:42Z

The @since Javadoc tags for the new classes and methods should all be @since 4.13. If we decide not to have a 4.13 and jump to 5.0, we'll go back and update them. Thanks.

kcooney · 2014-12-19T08:07:13Z

src/main/java/org/junit/rules/TemporaryFolder.java

+    /**
+     * Builds an instance of {@link TemporaryFolder}.
+     */
+    public static class Builder {


For the Timeout rule, we gave the Builder a protected constructor. I think the reason was because most users should use the builder() static method, but if someone wants to subclass Timeout they should be allowed to subclass the Builder.

Perhaps we should make this Builder have a protected constructor, too.

…ryFolder rule with test cases

npathai · 2014-12-19T09:56:38Z

@kcooney All done :)

marcphilipp · 2014-12-21T14:43:31Z

LGTM!

One last thing: Please add Javadoc to TemporaryFolder that mentions the new feature (assureDeletion) and shows how to use the builder.

marcphilipp · 2014-12-21T14:44:19Z

src/test/java/org/junit/tests/experimental/rules/TemporaryFolderRuleAssuredDeletionTest.java

+    public void testStrictVerificationFailure() {
+        PrintableResult result = testResult(HasTempFolderWithAssuredDeletion.class);
+        assertThat(result, failureCountIs(1));
+        assertThat(result.toString(), CoreMatchers.containsString("Unable to clean up temporary folder"));


Please add a static import for containsString.

…etion

npathai · 2014-12-22T10:56:08Z

@marcphilipp Done. Should I also add this feature to 4.13 release notes? Also we need to update the Rules wiki which still says that deletion is guaranteed.

marcphilipp · 2014-12-23T08:31:42Z

@NarendraPathai Thanks! Yes, we should update the release notes in the wiki after the merge and update the Rules wiki page (good catch!).

@kcooney Can you please take a look at the Javadoc as a native speaker?

kcooney · 2014-12-23T17:00:59Z

I'm on vacation, but I'll take a look at the Javadoc and merge this within a week.

Thanks for all of the work on this!

npathai · 2014-12-26T06:17:09Z

@kcooney @marcphilipp Thanks to you too for supporting.

kcooney · 2015-01-04T20:42:49Z

Did some very minor style cleanup, and some cleanup of the tests, and merged.

@NarendraPathai could you update the release notes? Thanks!

npathai · 2015-01-05T03:52:08Z

@kcooney Yes I will do it on Tuesday. Just a reminder that wiki section needs to be updated too.

npathai · 2015-01-08T04:59:00Z

@kcooney I have updated the release notes. Should I also update the wiki?

kcooney · 2015-01-08T16:06:34Z

@NarendraPathai yes please

marcphilipp · 2015-01-10T18:50:48Z

@NarendraPathai Please mention on the wiki page that the new feature will be available from 4.13 on.

npathai · 2015-01-12T06:17:54Z

@marcphilipp Yes for sure. I will add it today.

npathai · 2015-01-12T11:40:09Z

@kcooney @marcphilipp Done.

marcphilipp · 2015-01-12T18:45:51Z

Thanks!

kcooney reviewed Dec 10, 2014
View reviewed changes

kcooney self-assigned this Dec 10, 2014

npathai force-pushed the Issue1001 branch 2 times, most recently from fc4be6a to 982bb48 Compare December 15, 2014 06:05

kcooney reviewed Dec 15, 2014
View reviewed changes

npathai force-pushed the Issue1001 branch 3 times, most recently from 68864cb to 54dc9ca Compare December 16, 2014 08:15

npathai force-pushed the Issue1001 branch from 54dc9ca to 5ded953 Compare December 16, 2014 08:29

marcphilipp reviewed Dec 16, 2014
View reviewed changes

npathai force-pushed the Issue1001 branch from 5ded953 to 78fd299 Compare December 16, 2014 12:51

kcooney reviewed Dec 16, 2014
View reviewed changes

npathai force-pushed the Issue1001 branch from 78fd299 to 73539ab Compare December 17, 2014 05:04

npathai force-pushed the Issue1001 branch from 73539ab to 1093229 Compare December 19, 2014 07:26

kcooney reviewed Dec 19, 2014
View reviewed changes

Added support for strict verification of resource deletion in Tempora…

67149af

…ryFolder rule with test cases

npathai force-pushed the Issue1001 branch from 1093229 to 67149af Compare December 19, 2014 09:51

marcphilipp reviewed Dec 21, 2014
View reviewed changes

Added javadoc to temporaryfolder rule explaining the use of assureDel…

8a8f6c1

…etion

kcooney closed this in de92695 Jan 4, 2015

stefanbirkner added this to the 4.13 milestone Apr 13, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strict verification of resource deletion in TemporaryFolder rule #1044

Strict verification of resource deletion in TemporaryFolder rule #1044

npathai commented Dec 10, 2014

kcooney Dec 10, 2014

npathai commented Dec 15, 2014

kcooney commented Dec 15, 2014

kcooney Dec 15, 2014

npathai Dec 16, 2014

npathai commented Dec 16, 2014

marcphilipp Dec 16, 2014

marcphilipp Dec 16, 2014

kcooney Dec 16, 2014

marcphilipp commented Dec 16, 2014

npathai commented Dec 16, 2014

kcooney Dec 16, 2014

npathai commented Dec 17, 2014

kcooney commented Dec 19, 2014

kcooney Dec 19, 2014

npathai commented Dec 19, 2014

marcphilipp commented Dec 21, 2014

marcphilipp Dec 21, 2014

npathai commented Dec 22, 2014

marcphilipp commented Dec 23, 2014

kcooney commented Dec 23, 2014

npathai commented Dec 26, 2014

kcooney commented Jan 4, 2015

npathai commented Jan 5, 2015

npathai commented Jan 8, 2015

kcooney commented Jan 8, 2015

marcphilipp commented Jan 10, 2015

npathai commented Jan 12, 2015

npathai commented Jan 12, 2015

marcphilipp commented Jan 12, 2015

Strict verification of resource deletion in TemporaryFolder rule #1044

Strict verification of resource deletion in TemporaryFolder rule #1044

Conversation

npathai commented Dec 10, 2014

Choose a reason for hiding this comment

npathai commented Dec 15, 2014

kcooney commented Dec 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npathai commented Dec 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcphilipp commented Dec 16, 2014

Builder

Mutability

npathai commented Dec 16, 2014

Behavior

Choose a reason for hiding this comment

npathai commented Dec 17, 2014

kcooney commented Dec 19, 2014

Choose a reason for hiding this comment

npathai commented Dec 19, 2014

marcphilipp commented Dec 21, 2014

Choose a reason for hiding this comment

npathai commented Dec 22, 2014

marcphilipp commented Dec 23, 2014

kcooney commented Dec 23, 2014

npathai commented Dec 26, 2014

kcooney commented Jan 4, 2015

npathai commented Jan 5, 2015

npathai commented Jan 8, 2015

kcooney commented Jan 8, 2015

marcphilipp commented Jan 10, 2015

npathai commented Jan 12, 2015

npathai commented Jan 12, 2015

marcphilipp commented Jan 12, 2015