[JGIT RFC PATCH 3/3] Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

[JGIT RFC PATCH 3/3] Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

[Jonas Fonseca]

On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are
printed. In most cases the path is in fact deleted and it seems to just
be a timing bug or something Windows or NTFS specific. Also, many of the
warnings are for the same paths and therefore a bit redundant.
Furthermore, when a lot of warnings are printed it gives the feeling of
slowing down the tests. By rate limiting the warnings the problem is
still made visible for developers to notice.

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

 This is mostly an annoyance fix, but might be acceptable.

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index 9d7d133..44e24d7 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -51,6 +51,8 @@
 
 public abstract class RepositoryTestCase extends TestCase {
 
+	private static long deleteFailures;
+
 	protected final File trashParent = new File("trash");
 
 	protected File trash;
@@ -79,7 +81,7 @@ protected static void recursiveDelete(final File dir) {
 			}
 		}
 		dir.delete();
-		if (dir.exists()) {
+		if (dir.exists() && deleteFailures++ < 42) {
 			System.out.println("Warning: Failed to delete " + dir);
 		}
 	}
-- 
1.6.0.2.1166.g8d97a.dirty

-- 
Jonas Fonseca

Re: [JGIT RFC PATCH 3/3] Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

[Robin Rosenberg]

onsdagen den 22 oktober 2008 10.34.20 skrev Jonas Fonseca:
> On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are
> printed. In most cases the path is in fact deleted and it seems to just
> be a timing bug or something Windows or NTFS specific. Also, many of the
> warnings are for the same paths and therefore a bit redundant.
> Furthermore, when a lot of warnings are printed it gives the feeling of
> slowing down the tests. By rate limiting the warnings the problem is
> still made visible for developers to notice.

The problem is actually flaws in the unit tests and in the supporting RepositoryTestCase.
I think I'll fix that instead.

-- robin

Re: [JGIT RFC PATCH 3/3] Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

[Jonas Fonseca]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote Fri, Oct 24, 2008:
> onsdagen den 22 oktober 2008 10.34.20 skrev Jonas Fonseca:
> > On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are
> > printed. In most cases the path is in fact deleted and it seems to
> > just be a timing bug or something Windows or NTFS specific.
>
> The problem is actually flaws in the unit tests and in the supporting
> RepositoryTestCase.  I think I'll fix that instead.

Thanks. Any progress on this? Can I help in any way?

-- 
Jonas Fonseca

Re: [JGIT RFC PATCH 3/3] Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

[Robin Rosenberg]

torsdagen den 30 oktober 2008 11.46.20 skrev Jonas Fonseca:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote Fri, Oct 24, 2008:
> > onsdagen den 22 oktober 2008 10.34.20 skrev Jonas Fonseca:
> > > On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are
> > > printed. In most cases the path is in fact deleted and it seems to
> > > just be a timing bug or something Windows or NTFS specific.
> >
> > The problem is actually flaws in the unit tests and in the supporting
> > RepositoryTestCase.  I think I'll fix that instead.
> 
> Thanks. Any progress on this? Can I help in any way?

Ok here is a W-I-P. One test case is not cleaning up properly. You may note that I
still give some room for gc not to unmap immediately, but at the end of the program
we hope that will be the case and I think the missing case is just a bug and not GC
being delayed.  The counter testcount is only here for debugging this problem.

-- robin

>From b3907a11bec0158a99bc77a7655379725203da9c Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosnberg@dewire.com>
Date: Thu, 23 Oct 2008 01:27:50 +0200
Subject: [EGIT PATCH] Make the cleanup less verbose when it fails to delete temporary stuff.

---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   57 +++++++++++++++-----
 1 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index 9d7d133..8015a18 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -47,6 +47,7 @@
 import java.io.Reader;
 
 import junit.framework.TestCase;
+
 import org.spearce.jgit.util.JGitTestUtil;
 
 public abstract class RepositoryTestCase extends TestCase {
@@ -66,22 +67,34 @@
 		jcommitter = new PersonIdent("J. Committer", "jcommitter@example.com");
 	}
 
-	protected static void recursiveDelete(final File dir) {
+	protected static boolean recursiveDelete(final File dir) {
+		return recursiveDelete(dir, false);
+	}
+
+	protected static boolean recursiveDelete(final File dir, boolean silent) {
+		if (!dir.exists())
+			return silent;
 		final File[] ls = dir.listFiles();
 		if (ls != null) {
 			for (int k = 0; k < ls.length; k++) {
 				final File e = ls[k];
 				if (e.isDirectory()) {
-					recursiveDelete(e);
+					silent = recursiveDelete(e, silent);
 				} else {
-					e.delete();
+					if (!e.delete()) {
+						if (!silent)
+							System.out.println("Warning: Failed to delete " + e);
+						silent = true;
+					}
 				}
 			}
 		}
-		dir.delete();
-		if (dir.exists()) {
-			System.out.println("Warning: Failed to delete " + dir);
+		if (!dir.delete()) {
+			if (!silent)
+				System.out.println("Warning: Failed to delete " + dir);
+			silent = true;
 		}
+		return silent;
 	}
 
 	protected static void copyFile(final File src, final File dst)
@@ -121,19 +134,31 @@ protected static void checkFile(File f, final String checkData)
 
 	protected Repository db;
 
+	private static int testcount;
+	private static Thread shutdownhook;
+
 	public void setUp() throws Exception {
 		super.setUp();
-		recursiveDelete(trashParent);
-		trash = new File(trashParent,"trash"+System.currentTimeMillis());
+		System.gc();
+		trash = new File(trashParent,"xtrash"+System.currentTimeMillis()+"."+(testcount++));
+		recursiveDelete(trashParent, true);
 		trash_git = new File(trash, ".git");
-
+		if (shutdownhook == null) {
+			shutdownhook = new Thread() {
+				@Override
+				public void run() {
+					System.gc();
+					recursiveDelete(trashParent, false);
+				}
+			};
+			Runtime.getRuntime().addShutdownHook(shutdownhook);
+		}
 		Runtime.getRuntime().addShutdownHook(new Thread() {
 			@Override
 			public void run() {
-				recursiveDelete(trashParent);
+				recursiveDelete(trash);
 			}
 		});
-
 		db = new Repository(trash_git);
 		db.create();
 
@@ -170,12 +195,18 @@ protected void tearDown() throws Exception {
 	 * @throws IOException
 	 */
 	protected Repository createNewEmptyRepo() throws IOException {
-		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
+		final File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"."+(testcount++)+"/.git");
 		assertFalse(newTestRepo.exists());
-		File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis());
+		File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis()+"."+(testcount++));
 		assertTrue(unusedDir.mkdirs());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
+		Runtime.getRuntime().addShutdownHook(new Thread() {
+			@Override
+			public void run() {
+				recursiveDelete(newTestRepo);
+			}
+		});
 		return newRepo;
 	}
 
-- 
1.6.0.2.308.gef4a