[JGIT PATCH 0/4] RepositoryTestCase cleanups

[JGIT PATCH 0/4] RepositoryTestCase cleanups

[Robin Rosenberg]

Ok, so here is an attempt to improve the ability of the JGit's unit
tests to delete temporary repositories. This has probably been seen
by many, but Jonas Fonseca raised the issue.

The background is that on Windows you cannot delete files that are
open and mmapped files are open until they get unmapped, which in
Java is beyond explicit programmer control. You can only free the
resources and pray that the GC does the work. Fortunately it usually
does. It turned out our testcases weren't even trying to clean up
properly. 

-- robin

Robin Rosenberg (4):
  Make the cleanup less verbose when it fails to delete temporary
    stuff.
  Add shutdown hooks to try to clean up after unit tests anyway
  Cleanup malformed test cases
  Automatically clean up any repositories created by the test cases

 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |    3 +
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   82 +++++++++++++++++---
 2 files changed, 73 insertions(+), 12 deletions(-)

Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Ok, so here is an attempt to improve the ability of the JGit's unit
> tests to delete temporary repositories. This has probably been seen
> by many, but Jonas Fonseca raised the issue.

Hmpph.  This takes 19 seconds to run the suite, where it used to be
only 2 seconds on the same system.  The slower run isn't something
I'm too happy about, actually I'd like to make the run even faster
than 2 seconds.
 
> The background is that on Windows you cannot delete files that are
> open and mmapped files are open until they get unmapped, which in
> Java is beyond explicit programmer control. You can only free the
> resources and pray that the GC does the work. Fortunately it usually
> does. It turned out our testcases weren't even trying to clean up
> properly. 

If the issue is mmap'd files, why don't we instead disable mmap
on Windows during JUnit tests, and use the non-mmap variant of
pack access?  At least do that for the bulk of the tests, and
then have a single test case which tests the mmap code path but
has careful System.gc calls in place to try and ensure we can
actually clean up the temporary files.

Another option is to refactor the Repository class a little so we
can replace local filesystem IO with something else, like say an
in-core repository.  E.g. a pack file and/or pack index stored in
a byte[], and refs stored in a HashMap.  We'd still need a couple
of tests to verify local disk IO, but many of the tests can be
validated against such a pure in-memory Repository concept.

I'd actually like to get that Repository refactoring done soon,
someone else was asking about it for the RefDatabase (to store the
refs in a SQL database so JGit ties into JTA) but I may also want
it for Gerrit 2 - I'm looking at doing something that would put
200,000 refs per year into a repository.  That's so large that
most operations can't afford to scan the entire ref database,
and it really cannot be loose.  ;-)


Refactoring repository is a fair chunk of work, disabling the mmap
feature under Windows in JUnit may be easier.  Hmm, according to
WindowCache's <clinit> its default by false.  Why is it enabling
on Windows?  The only code that calls WindowCache.reconfigure()
is in the Eclipse plugin, so pure JGit unit tests shouldn't be
turning on mmap code *at all*.

Which also points out a gap in our tests.  Nothing new, we have
lots of gaps.  *sigh*

-- 
Shawn.

Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups

[Robin Rosenberg]

torsdag 27 november 2008 22:49:16 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > Ok, so here is an attempt to improve the ability of the JGit's unit
> > tests to delete temporary repositories. This has probably been seen
> > by many, but Jonas Fonseca raised the issue.
> 
> Hmpph.  This takes 19 seconds to run the suite, where it used to be
> only 2 seconds on the same system.  The slower run isn't something
> I'm too happy about, actually I'd like to make the run even faster
> than 2 seconds.

So maybe we should clean up less. Every new test repo we create has
a new name so we could do without cleaning up so much. The cleanup
however is a verification that we close (and can close) our resources, 
though it only works on Windows :/ On unix we could spawn lsof but that
is really really slow. 

> If the issue is mmap'd files, why don't we instead disable mmap
> on Windows during JUnit tests, and use the non-mmap variant of
> pack access?  At least do that for the bulk of the tests, and
> then have a single test case which tests the mmap code path but
> has careful System.gc calls in place to try and ensure we can
> actually clean up the temporary files.
We would then need some other really slow test to play rough with
memory mapping and gc. As I mentioned above it is actually about
closing resources in general, mmapped files being an especially 
nasty case.

> I'd actually like to get that Repository refactoring done soon,
> someone else was asking about it for the RefDatabase (to store the
> refs in a SQL database so JGit ties into JTA) but I may also want
> it for Gerrit 2 - I'm looking at doing something that would put
> 200,000 refs per year into a repository.  That's so large that
> most operations can't afford to scan the entire ref database,
> and it really cannot be loose.  ;-)

Would be cool, but having that diff engine is more important to me.

> 
> 
> Refactoring repository is a fair chunk of work, disabling the mmap
> feature under Windows in JUnit may be easier.  Hmm, according to
> WindowCache's <clinit> its default by false.  Why is it enabling
> on Windows?  The only code that calls WindowCache.reconfigure()
> is in the Eclipse plugin, so pure JGit unit tests shouldn't be
> turning on mmap code *at all*.
> 
> Which also points out a gap in our tests.  Nothing new, we have
> lots of gaps.  *sigh*

Yep.  :/

-- robin

Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups

[Johannes Schindelin]

Hi,

On Sat, 29 Nov 2008, Robin Rosenberg wrote:

> [Repository refactoring] Would be cool, but having that diff engine is 
> more important to me.

Stay tuned.  I have something that outputs something resembling a diff 
now.  Of course, the output is not correct yet, due to bugs I introduced 
cunnily when trying to fix another bug.

I'll keep you posted,
Dscho

Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups

[Robin Rosenberg]

torsdag 27 november 2008 22:49:16 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > Ok, so here is an attempt to improve the ability of the JGit's unit
> > tests to delete temporary repositories. This has probably been seen
> > by many, but Jonas Fonseca raised the issue.
> 
> Hmpph.  This takes 19 seconds to run the suite, where it used to be
> only 2 seconds on the same system.  The slower run isn't something
> I'm too happy about, actually I'd like to make the run even faster
> than 2 seconds.

Ok, that's the GC calls, I added. I'll post a completely new set of patches
disabling mmap by default, overridable using system properties.

-- robin

[JGIT PATCH v2 0/8] Unit test cleanups

[Robin Rosenberg]

A completele reworked set of patches, including fixing a couple
more forgot-to-close bugs and Shawns suggestion that we disable
memory mapping in junit tests by default.

-- robin

Robin Rosenberg (8):
  Drop unneeded code in unit tests
  Cleanup malformed test cases
  Turn off memory mapping in JGit unit tests by default
  Add a counter to make sure the test repo name is unique
  Make the cleanup less verbose when it fails to delete temporary
    stuff.
  Cleanup after each test.
  Close files opened by unit testing framework
  Hard failure on unit test cleanups if they fail.

 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |    3 +
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |  152 +++++++++++++++++---
 .../tst/org/spearce/jgit/lib/T0007_Index.java      |   10 +-
 3 files changed, 139 insertions(+), 26 deletions(-)

[JGIT PATCH 1/8] Drop unneeded code in unit tests

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    2 --
 1 files changed, 0 insertions(+), 2 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..e164faf 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
@@ -172,8 +172,6 @@ protected void tearDown() throws Exception {
 	protected Repository createNewEmptyRepo() throws IOException {
 		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
 		assertFalse(newTestRepo.exists());
-		File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis());
-		assertTrue(unusedDir.mkdirs());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
 		return newRepo;
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 2/8] Cleanup malformed test cases

[Robin Rosenberg]

These were abusing setup, resulting in lost resources. To
fix this we can abuse tearDown too.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
index e5bce4d..3c02955 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
@@ -309,6 +309,7 @@ public void testWritePack4ThinPack() throws IOException {
 	public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
 		testWritePack2();
 		final int sizePack2NoDeltas = cos.getCount();
+		tearDown();
 		setUp();
 		testWritePack2DeltasReuseRefs();
 		final int sizePack2DeltasRefs = cos.getCount();
@@ -327,6 +328,7 @@ public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
 	public void testWritePack2SizeOffsetsVsRefs() throws Exception {
 		testWritePack2DeltasReuseRefs();
 		final int sizePack2DeltasRefs = cos.getCount();
+		tearDown();
 		setUp();
 		testWritePack2DeltasReuseOffsets();
 		final int sizePack2DeltasOffsets = cos.getCount();
@@ -344,6 +346,7 @@ public void testWritePack2SizeOffsetsVsRefs() throws Exception {
 	public void testWritePack4SizeThinVsNoThin() throws Exception {
 		testWritePack4();
 		final int sizePack4 = cos.getCount();
+		tearDown();
 		setUp();
 		testWritePack4ThinPack();
 		final int sizePack4Thin = cos.getCount();
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 3/8] Turn off memory mapping in JGit unit tests by default

[Robin Rosenberg]

A system property named jgit.junit.usemmmap can be set to true to enable
memory mapping during unit testing.

The protected method configure can be overridden to do things
like configuring the JGit engine.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   30 ++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 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 e164faf..3b08fa5 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
@@ -49,6 +49,19 @@
 import junit.framework.TestCase;
 import org.spearce.jgit.util.JGitTestUtil;
 
+/**
+ * Base class for most JGit unit tests.
+ *
+ * Sets up a predefined test repository and has support for creating additional
+ * repositories and destroying them when the tests are finished.
+ *
+ * A system property <em>jgit.junit.usemmmap</em> defines whether memory mapping
+ * is used. Memory mapping has an effect on the file system, in that memory
+ * mapped files in java cannot be deleted as long as they mapped arrays have not
+ * been reclaimed by the garbage collector. The programmer cannot control this
+ * with precision, though hinting using <em>{@link java.lang.System#gc}</em>
+ * often helps.
+ */
 public abstract class RepositoryTestCase extends TestCase {
 
 	protected final File trashParent = new File("trash");
@@ -66,6 +79,22 @@
 		jcommitter = new PersonIdent("J. Committer", "jcommitter@example.com");
 	}
 
+	protected boolean packedGitMMAP;
+
+	/**
+	 * Configure JGit before setting up test repositories.
+	 */
+	protected void configure() {
+		packedGitMMAP = "true".equals(System.getProperty("jgit.junit.usemmmap"));
+		WindowCache.reconfigure(128*1024, 8192, packedGitMMAP, 8192);
+	}
+
+	/**
+	 * Utility method to delete a directory recursively. It is
+	 * also used internally.
+	 *
+	 * @param dir
+	 */
 	protected static void recursiveDelete(final File dir) {
 		final File[] ls = dir.listFiles();
 		if (ls != null) {
@@ -123,6 +152,7 @@ protected static void checkFile(File f, final String checkData)
 
 	public void setUp() throws Exception {
 		super.setUp();
+		configure();
 		recursiveDelete(trashParent);
 		trash = new File(trashParent,"trash"+System.currentTimeMillis());
 		trash_git = new File(trash, ".git");
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 4/8] Add a counter to make sure the test repo name is unique

[Robin Rosenberg]

System.currentTimeMillis() does not have the granularity
necessary to guarantee uniqueness. We keep it to make sure we
have unique names between different runs, but add a counter to
make it unique within the execution of a test suite.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    6 ++++--
 1 files changed, 4 insertions(+), 2 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 3b08fa5..6ea9b45 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
@@ -150,11 +150,13 @@ protected static void checkFile(File f, final String checkData)
 
 	protected Repository db;
 
+	private static int testcount;
+
 	public void setUp() throws Exception {
 		super.setUp();
 		configure();
 		recursiveDelete(trashParent);
-		trash = new File(trashParent,"trash"+System.currentTimeMillis());
+		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
 
 		Runtime.getRuntime().addShutdownHook(new Thread() {
@@ -200,7 +202,7 @@ protected void tearDown() throws Exception {
 	 * @throws IOException
 	 */
 	protected Repository createNewEmptyRepo() throws IOException {
-		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
+		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"."+(testcount++)+"/.git");
 		assertFalse(newTestRepo.exists());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 5/8] Make the cleanup less verbose when it fails to delete temporary stuff.

[Robin Rosenberg]

Pass the test case name for easier tracking of the test case that
causes problems.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   36 ++++++++++++++++----
 1 files changed, 29 insertions(+), 7 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 6ea9b45..8e23bc1 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
@@ -96,21 +96,42 @@ protected void configure() {
 	 * @param dir
 	 */
 	protected static void recursiveDelete(final File dir) {
+		recursiveDelete(dir, false, null);
+	}
+
+	protected static boolean recursiveDelete(final File dir, boolean silent,
+			final String name) {
+		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, name);
 				} else {
-					e.delete();
+					if (!e.delete()) {
+						if (!silent) {
+							String msg = "Warning: Failed to delete " + e;
+							if (name != null)
+								msg += " in " + name;
+							System.out.println(msg);
+						}
+						silent = true;
+					}
 				}
 			}
 		}
-		dir.delete();
-		if (dir.exists()) {
-			System.out.println("Warning: Failed to delete " + dir);
+		if (!dir.delete()) {
+			if (!silent) {
+				String msg = "Warning: Failed to delete " + dir;
+				if (name != null)
+					msg += " in " + name;
+				System.out.println(msg);
+			}
+			silent = true;
 		}
+		return silent;
 	}
 
 	protected static void copyFile(final File src, final File dst)
@@ -155,14 +176,15 @@ protected static void checkFile(File f, final String checkData)
 	public void setUp() throws Exception {
 		super.setUp();
 		configure();
-		recursiveDelete(trashParent);
+		final String name = getClass().getName() + "." + getName();
+		recursiveDelete(trashParent, true, name);
 		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
 
 		Runtime.getRuntime().addShutdownHook(new Thread() {
 			@Override
 			public void run() {
-				recursiveDelete(trashParent);
+				recursiveDelete(trashParent, false, name);
 			}
 		});
 
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 6/8] Cleanup after each test.

[Robin Rosenberg]

Automatically clean up any repositories created by the test cases.
Cleanup is attempted at the end of each test, but if that fails
Shutdown hooks attempt to clean up when the JVM exits. If memmory
mapping is enabled (disabled by default in unit tests), gc is
invoked to make it more likely that cleanup will occur successfully.
The drawback is that this is much slower, which is the reason we
disble memory mapping by default in unit tests.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   62 ++++++++++++++++----
 1 files changed, 50 insertions(+), 12 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 8e23bc1..aaa3592 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
@@ -45,6 +45,8 @@
 import java.io.InputStreamReader;
 import java.io.OutputStreamWriter;
 import java.io.Reader;
+import java.util.ArrayList;
+import java.util.List;
 
 import junit.framework.TestCase;
 import org.spearce.jgit.util.JGitTestUtil;
@@ -95,8 +97,8 @@ protected void configure() {
 	 *
 	 * @param dir
 	 */
-	protected static void recursiveDelete(final File dir) {
-		recursiveDelete(dir, false, null);
+	protected void recursiveDelete(final File dir) {
+		recursiveDelete(dir, false, getClass().getName() + "." + getName());
 	}
 
 	protected static boolean recursiveDelete(final File dir, boolean silent,
@@ -170,9 +172,12 @@ protected static void checkFile(File f, final String checkData)
 	}
 
 	protected Repository db;
-
+	private static Thread shutdownhook;
+	private static List<Runnable> shutDownCleanups = new ArrayList<Runnable>();
 	private static int testcount;
 
+	private ArrayList<Repository> repositoriesToClose = new ArrayList<Repository>();
+
 	public void setUp() throws Exception {
 		super.setUp();
 		configure();
@@ -180,14 +185,23 @@ public void setUp() throws Exception {
 		recursiveDelete(trashParent, true, name);
 		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
-
-		Runtime.getRuntime().addShutdownHook(new Thread() {
-			@Override
-			public void run() {
-				recursiveDelete(trashParent, false, name);
-			}
-		});
-
+		if (shutdownhook == null) {
+			shutdownhook = new Thread() {
+				@Override
+				public void run() {
+					// This may look superfluous, but is an extra attempt
+					// to clean up. First GC to release as many resources
+					// as possible and then try to clean up one test repo
+					// at a time (to record problems) and finally to drop
+					// the directory containing all test repositories.
+					System.gc();
+					for (Runnable r : shutDownCleanups)
+						r.run();
+					recursiveDelete(trashParent, false, null);
+				}
+			};
+			Runtime.getRuntime().addShutdownHook(shutdownhook);
+		}
 		db = new Repository(trash_git);
 		db.create();
 
@@ -213,6 +227,22 @@ copyFile(JGitTestUtil.getTestResourceFile(packs[k] + ".idx"), new File(packDir,
 
 	protected void tearDown() throws Exception {
 		db.close();
+		for (Repository r : repositoriesToClose)
+			r.close();
+
+		// Since memory mapping is controlled by the GC we need to
+		// tell it this is a good time to clean up and unlock
+		// memory mapped files.
+		if (packedGitMMAP)
+			System.gc();
+
+		final String name = getClass().getName() + "." + getName();
+		recursiveDelete(trash, false, name);
+		for (Repository r : repositoriesToClose)
+			recursiveDelete(r.getWorkDir(), false, name);
+
+		repositoriesToClose.clear();
+
 		super.tearDown();
 	}
 
@@ -224,10 +254,18 @@ protected void tearDown() throws Exception {
 	 * @throws IOException
 	 */
 	protected Repository createNewEmptyRepo() throws IOException {
-		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"."+(testcount++)+"/.git");
+		final File newTestRepo = new File(trashParent, "new"
+				+ System.currentTimeMillis() + "." + (testcount++) + "/.git");
 		assertFalse(newTestRepo.exists());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
+		final String name = getClass().getName() + "." + getName();
+		shutDownCleanups.add(new Runnable() {
+			public void run() {
+				recursiveDelete(newTestRepo, false, name);
+			}
+		});
+		repositoriesToClose.add(newRepo);
 		return newRepo;
 	}
 
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 7/8] Close files opened by unit testing framework

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   12 ++++++++----
 .../tst/org/spearce/jgit/lib/T0007_Index.java      |   10 +++++++---
 2 files changed, 15 insertions(+), 7 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 aaa3592..376a76e 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
@@ -165,10 +165,14 @@ protected File writeTrashFile(final String name, final String data)
 	protected static void checkFile(File f, final String checkData)
 			throws IOException {
 		Reader r = new InputStreamReader(new FileInputStream(f), "ISO-8859-1");
-		char[] data = new char[(int) f.length()];
-		if (f.length() !=  r.read(data))
-			throw new IOException("Internal error reading file data from "+f);
-		assertEquals(checkData, new String(data));
+		try {
+			char[] data = new char[(int) f.length()];
+			if (f.length() !=  r.read(data))
+				throw new IOException("Internal error reading file data from "+f);
+			assertEquals(checkData, new String(data));
+		} finally {
+			r.close();
+		}
 	}
 
 	protected Repository db;
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0007_Index.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0007_Index.java
index 69f3a48..499812e 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0007_Index.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0007_Index.java
@@ -424,9 +424,13 @@ public void test031_executeBit_coreModeFalse() throws IllegalArgumentException, 
 	private String content(File f) throws IOException {
 		byte[] buf = new byte[(int) f.length()];
 		FileInputStream is = new FileInputStream(f);
-		int read = is.read(buf);
-		assertEquals(f.length(), read);
-		return new String(buf, 0);
+		try {
+			int read = is.read(buf);
+			assertEquals(f.length(), read);
+			return new String(buf, 0);
+		} finally {
+			is.close();
+		}
 	}
 
 	private void delete(File f) throws IOException {
-- 
1.6.0.3.640.g6331a

[JGIT PATCH 8/8] Hard failure on unit test cleanups if they fail.

[Robin Rosenberg]

This only has an effect on Windows that locks open files, but
is a nice test that we actually clean up.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   52 ++++++++++++--------
 1 files changed, 32 insertions(+), 20 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 376a76e..22bf395 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
@@ -49,6 +49,7 @@
 import java.util.List;
 
 import junit.framework.TestCase;
+
 import org.spearce.jgit.util.JGitTestUtil;
 
 /**
@@ -93,16 +94,18 @@ protected void configure() {
 
 	/**
 	 * Utility method to delete a directory recursively. It is
-	 * also used internally.
+	 * also used internally. If a file or directory cannote be removed
+	 * it throws an AssertionFailure.
 	 *
 	 * @param dir
 	 */
 	protected void recursiveDelete(final File dir) {
-		recursiveDelete(dir, false, getClass().getName() + "." + getName());
+		recursiveDelete(dir, false, getClass().getName() + "." + getName(), true);
 	}
 
 	protected static boolean recursiveDelete(final File dir, boolean silent,
-			final String name) {
+			final String name, boolean failOnError) {
+		assert !(silent && failOnError);
 		if (!dir.exists())
 			return silent;
 		final File[] ls = dir.listFiles();
@@ -110,32 +113,42 @@ protected static boolean recursiveDelete(final File dir, boolean silent,
 			for (int k = 0; k < ls.length; k++) {
 				final File e = ls[k];
 				if (e.isDirectory()) {
-					silent = recursiveDelete(e, silent, name);
+					silent = recursiveDelete(e, silent, name, failOnError);
 				} else {
 					if (!e.delete()) {
 						if (!silent) {
-							String msg = "Warning: Failed to delete " + e;
-							if (name != null)
-								msg += " in " + name;
-							System.out.println(msg);
+							reportDeleteFailure(name, failOnError, e);
 						}
-						silent = true;
+						silent = !failOnError;
 					}
 				}
 			}
 		}
 		if (!dir.delete()) {
 			if (!silent) {
-				String msg = "Warning: Failed to delete " + dir;
-				if (name != null)
-					msg += " in " + name;
-				System.out.println(msg);
+				reportDeleteFailure(name, failOnError, dir);
 			}
-			silent = true;
+			silent = !failOnError;
 		}
 		return silent;
 	}
 
+	private static void reportDeleteFailure(final String name,
+			boolean failOnError, final File e) {
+		String severity;
+		if (failOnError)
+			severity = "Error";
+		else
+			severity = "Warning";
+		String msg = severity + ": Failed to delete " + e;
+		if (name != null)
+			msg += " in " + name;
+		if (failOnError)
+			fail(msg);
+		else
+			System.out.println(msg);
+	}
+
 	protected static void copyFile(final File src, final File dst)
 			throws IOException {
 		final FileInputStream fis = new FileInputStream(src);
@@ -186,7 +199,7 @@ public void setUp() throws Exception {
 		super.setUp();
 		configure();
 		final String name = getClass().getName() + "." + getName();
-		recursiveDelete(trashParent, true, name);
+		recursiveDelete(trashParent, true, name, false); // Cleanup old failed stuff
 		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
 		if (shutdownhook == null) {
@@ -201,7 +214,7 @@ public void run() {
 					System.gc();
 					for (Runnable r : shutDownCleanups)
 						r.run();
-					recursiveDelete(trashParent, false, null);
+					recursiveDelete(trashParent, false, null, false);
 				}
 			};
 			Runtime.getRuntime().addShutdownHook(shutdownhook);
@@ -241,10 +254,9 @@ protected void tearDown() throws Exception {
 			System.gc();
 
 		final String name = getClass().getName() + "." + getName();
-		recursiveDelete(trash, false, name);
+		recursiveDelete(trash, false, name, true);
 		for (Repository r : repositoriesToClose)
-			recursiveDelete(r.getWorkDir(), false, name);
-
+			recursiveDelete(r.getWorkDir(), false, name, true);
 		repositoriesToClose.clear();
 
 		super.tearDown();
@@ -266,7 +278,7 @@ protected Repository createNewEmptyRepo() throws IOException {
 		final String name = getClass().getName() + "." + getName();
 		shutDownCleanups.add(new Runnable() {
 			public void run() {
-				recursiveDelete(newTestRepo, false, name);
+				recursiveDelete(newTestRepo, false, name, false);
 			}
 		});
 		repositoriesToClose.add(newRepo);
-- 
1.6.0.3.640.g6331a

Re: [JGIT PATCH v2 0/8] Unit test cleanups

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> A completele reworked set of patches, including fixing a couple
> more forgot-to-close bugs and Shawns suggestion that we disable
> memory mapping in junit tests by default.
...
> Robin Rosenberg (8):
>   Drop unneeded code in unit tests
>   Cleanup malformed test cases
>   Turn off memory mapping in JGit unit tests by default
>   Add a counter to make sure the test repo name is unique
>   Make the cleanup less verbose when it fails to delete temporary
>     stuff.
>   Cleanup after each test.
>   Close files opened by unit testing framework
>   Hard failure on unit test cleanups if they fail.
> 
>  .../tst/org/spearce/jgit/lib/PackWriterTest.java   |    3 +
>  .../org/spearce/jgit/lib/RepositoryTestCase.java   |  152 +++++++++++++++++---
>  .../tst/org/spearce/jgit/lib/T0007_Index.java      |   10 +-
>  3 files changed, 139 insertions(+), 26 deletions(-)

Merged.  The series looked really good to me.  Second time is the
charm, eh?  :-)

-- 
Shawn.

[JGIT PATCH 0/4] RepositoryTestCase cleanups

[Robin Rosenberg]

Ok, so here is an attempt to improve the ability of the JGit's unit
tests to delete temporary repositories. This has probably been seen
by many, but Jonas Fonseca raised the issue.

The background is that on Windows you cannot delete files that are
open and mmapped files are open until they get unmapped, which in
Java is beyond explicit programmer control. You can only free the
resources and pray that the GC does the work. Fortunately it usually
does. It turned out our testcases weren't even trying to clean up
properly. 

-- robin

Robin Rosenberg (4):
  Make the cleanup less verbose when it fails to delete temporary
    stuff.
  Add shutdown hooks to try to clean up after unit tests anyway
  Cleanup malformed test cases
  Automatically clean up any repositories created by the test cases

 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |    3 +
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   82 +++++++++++++++++---
 2 files changed, 73 insertions(+), 12 deletions(-)