{"thread":{"id":"16509","subject":"[JGIT PATCH 0/4] RepositoryTestCase cleanups","startedAt":"2008-11-27T21:13:26Z","lastAt":"2008-12-02T16:38:56Z","messageCount":15,"participants":["Robin Rosenberg","Shawn O. Pearce","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"96640","messageId":"1227820410-9621-1-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":null,"subject":"[JGIT PATCH 0/4] RepositoryTestCase cleanups","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-27T21:13:26Z","receivedAt":"2008-11-27T21:13:26Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Ok, so here is an attempt to improve the ability of the JGit's unit\ntests to delete temporary repositories. This has probably been seen\nby many, but Jonas Fonseca raised the issue.\n\nThe background is that on Windows you cannot delete files that are\nopen and mmapped files are open until they get unmapped, which in\nJava is beyond explicit programmer control. You can only free the\nresources and pray that the GC does the work. Fortunately it usually\ndoes. It turned out our testcases weren't even trying to clean up\nproperly. \n\n-- robin\n\nRobin Rosenberg (4):\n Make the cleanup less verbose when it fails to delete temporary\n stuff.\n Add shutdown hooks to try to clean up after unit tests anyway\n Cleanup malformed test cases\n Automatically clean up any repositories created by the test cases\n\n .../tst/org/spearce/jgit/lib/PackWriterTest.java | 3 +\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 82 +++++++++++++++++---\n 2 files changed, 73 insertions(+), 12 deletions(-)\n"},{"id":"96651","messageId":"20081127214916.GD23984@spearce.org","threadId":"16509","inReplyTo":"1227820410-9621-1-git-send-email-robin.rosenberg@dewire.com","subject":"Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-11-27T21:49:16Z","receivedAt":"2008-11-27T21:49:16Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg wrote:\n> Ok, so here is an attempt to improve the ability of the JGit's unit\n> tests to delete temporary repositories. This has probably been seen\n> by many, but Jonas Fonseca raised the issue.\n\nHmpph. This takes 19 seconds to run the suite, where it used to be\nonly 2 seconds on the same system. The slower run isn't something\nI'm too happy about, actually I'd like to make the run even faster\nthan 2 seconds.\n \n> The background is that on Windows you cannot delete files that are\n> open and mmapped files are open until they get unmapped, which in\n> Java is beyond explicit programmer control. You can only free the\n> resources and pray that the GC does the work. Fortunately it usually\n> does. It turned out our testcases weren't even trying to clean up\n> properly. \n\nIf the issue is mmap'd files, why don't we instead disable mmap\non Windows during JUnit tests, and use the non-mmap variant of\npack access? At least do that for the bulk of the tests, and\nthen have a single test case which tests the mmap code path but\nhas careful System.gc calls in place to try and ensure we can\nactually clean up the temporary files.\n\nAnother option is to refactor the Repository class a little so we\ncan replace local filesystem IO with something else, like say an\nin-core repository. E.g. a pack file and/or pack index stored in\na byte[], and refs stored in a HashMap. We'd still need a couple\nof tests to verify local disk IO, but many of the tests can be\nvalidated against such a pure in-memory Repository concept.\n\nI'd actually like to get that Repository refactoring done soon,\nsomeone else was asking about it for the RefDatabase (to store the\nrefs in a SQL database so JGit ties into JTA) but I may also want\nit for Gerrit 2 - I'm looking at doing something that would put\n200,000 refs per year into a repository. That's so large that\nmost operations can't afford to scan the entire ref database,\nand it really cannot be loose. ;-)\n\n\nRefactoring repository is a fair chunk of work, disabling the mmap\nfeature under Windows in JUnit may be easier. Hmm, according to\nWindowCache's its default by false. Why is it enabling\non Windows? The only code that calls WindowCache.reconfigure()\nis in the Eclipse plugin, so pure JGit unit tests shouldn't be\nturning on mmap code *at all*.\n\nWhich also points out a gap in our tests. Nothing new, we have\nlots of gaps. *sigh*\n\n-- \nShawn.\n"},{"id":"96718","messageId":"200811291301.12095.robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"20081127214916.GD23984@spearce.org","subject":"Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-29T12:01:11Z","receivedAt":"2008-11-29T12:01:11Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"torsdag 27 november 2008 22:49:16 skrev Shawn O. Pearce:\n> Robin Rosenberg wrote:\n> > Ok, so here is an attempt to improve the ability of the JGit's unit\n> > tests to delete temporary repositories. This has probably been seen\n> > by many, but Jonas Fonseca raised the issue.\n> \n> Hmpph. This takes 19 seconds to run the suite, where it used to be\n> only 2 seconds on the same system. The slower run isn't something\n> I'm too happy about, actually I'd like to make the run even faster\n> than 2 seconds.\n\nSo maybe we should clean up less. Every new test repo we create has\na new name so we could do without cleaning up so much. The cleanup\nhowever is a verification that we close (and can close) our resources, \nthough it only works on Windows :/ On unix we could spawn lsof but that\nis really really slow. \n\n> If the issue is mmap'd files, why don't we instead disable mmap\n> on Windows during JUnit tests, and use the non-mmap variant of\n> pack access? At least do that for the bulk of the tests, and\n> then have a single test case which tests the mmap code path but\n> has careful System.gc calls in place to try and ensure we can\n> actually clean up the temporary files.\nWe would then need some other really slow test to play rough with\nmemory mapping and gc. As I mentioned above it is actually about\nclosing resources in general, mmapped files being an especially \nnasty case.\n\n> I'd actually like to get that Repository refactoring done soon,\n> someone else was asking about it for the RefDatabase (to store the\n> refs in a SQL database so JGit ties into JTA) but I may also want\n> it for Gerrit 2 - I'm looking at doing something that would put\n> 200,000 refs per year into a repository. That's so large that\n> most operations can't afford to scan the entire ref database,\n> and it really cannot be loose. ;-)\n\nWould be cool, but having that diff engine is more important to me.\n\n> \n> \n> Refactoring repository is a fair chunk of work, disabling the mmap\n> feature under Windows in JUnit may be easier. Hmm, according to\n> WindowCache's its default by false. Why is it enabling\n> on Windows? The only code that calls WindowCache.reconfigure()\n> is in the Eclipse plugin, so pure JGit unit tests shouldn't be\n> turning on mmap code *at all*.\n> \n> Which also points out a gap in our tests. Nothing new, we have\n> lots of gaps. *sigh*\n\nYep. :/\n\n-- robin\n"},{"id":"96783","messageId":"200811301518.09075.robin.rosenberg.lists@dewire.com","threadId":"16509","inReplyTo":"20081127214916.GD23984@spearce.org","subject":"Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-11-30T14:18:08Z","receivedAt":"2008-11-30T14:18:08Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"torsdag 27 november 2008 22:49:16 skrev Shawn O. Pearce:\n> Robin Rosenberg wrote:\n> > Ok, so here is an attempt to improve the ability of the JGit's unit\n> > tests to delete temporary repositories. This has probably been seen\n> > by many, but Jonas Fonseca raised the issue.\n> \n> Hmpph. This takes 19 seconds to run the suite, where it used to be\n> only 2 seconds on the same system. The slower run isn't something\n> I'm too happy about, actually I'd like to make the run even faster\n> than 2 seconds.\n\nOk, that's the GC calls, I added. I'll post a completely new set of patches\ndisabling mmap by default, overridable using system properties.\n\n-- robin\n"},{"id":"96796","messageId":"1228088435-23722-1-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"20081127214916.GD23984@spearce.org","subject":"[JGIT PATCH v2 0/8] Unit test cleanups","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:27Z","receivedAt":"2008-11-30T23:40:27Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"A completele reworked set of patches, including fixing a couple\nmore forgot-to-close bugs and Shawns suggestion that we disable\nmemory mapping in junit tests by default.\n\n-- robin\n\nRobin Rosenberg (8):\n Drop unneeded code in unit tests\n Cleanup malformed test cases\n Turn off memory mapping in JGit unit tests by default\n Add a counter to make sure the test repo name is unique\n Make the cleanup less verbose when it fails to delete temporary\n stuff.\n Cleanup after each test.\n Close files opened by unit testing framework\n Hard failure on unit test cleanups if they fail.\n\n .../tst/org/spearce/jgit/lib/PackWriterTest.java | 3 +\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 152 +++++++++++++++++---\n .../tst/org/spearce/jgit/lib/T0007_Index.java | 10 +-\n 3 files changed, 139 insertions(+), 26 deletions(-)\n"},{"id":"96797","messageId":"1228088435-23722-2-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-1-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 1/8] Drop unneeded code in unit tests","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:28Z","receivedAt":"2008-11-30T23:40:28Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Signed-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --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\nindex 9d7d133..e164faf 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -172,8 +172,6 @@ protected void tearDown() throws Exception {\n \tprotected Repository createNewEmptyRepo() throws IOException {\n \t\tFile newTestRepo = new File(trashParent, \"new\"+System.currentTimeMillis()+\"/.git\");\n \t\tassertFalse(newTestRepo.exists());\n-\t\tFile unusedDir = new File(trashParent, \"tmp\"+System.currentTimeMillis());\n-\t\tassertTrue(unusedDir.mkdirs());\n \t\tfinal Repository newRepo = new Repository(newTestRepo);\n \t\tnewRepo.create();\n \t\treturn newRepo;\n-- \n1.6.0.3.640.g6331a\n"},{"id":"96802","messageId":"1228088435-23722-3-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-2-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 2/8] Cleanup malformed test cases","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:29Z","receivedAt":"2008-11-30T23:40:29Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"These were abusing setup, resulting in lost resources. To\nfix this we can abuse tearDown too.\n\nSigned-off-by: Robin Rosenberg \n---\n .../tst/org/spearce/jgit/lib/PackWriterTest.java | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --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\nindex e5bce4d..3c02955 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java\n@@ -309,6 +309,7 @@ public void testWritePack4ThinPack() throws IOException {\n \tpublic void testWritePack2SizeDeltasVsNoDeltas() throws Exception {\n \t\ttestWritePack2();\n \t\tfinal int sizePack2NoDeltas = cos.getCount();\n+\t\ttearDown();\n \t\tsetUp();\n \t\ttestWritePack2DeltasReuseRefs();\n \t\tfinal int sizePack2DeltasRefs = cos.getCount();\n@@ -327,6 +328,7 @@ public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {\n \tpublic void testWritePack2SizeOffsetsVsRefs() throws Exception {\n \t\ttestWritePack2DeltasReuseRefs();\n \t\tfinal int sizePack2DeltasRefs = cos.getCount();\n+\t\ttearDown();\n \t\tsetUp();\n \t\ttestWritePack2DeltasReuseOffsets();\n \t\tfinal int sizePack2DeltasOffsets = cos.getCount();\n@@ -344,6 +346,7 @@ public void testWritePack2SizeOffsetsVsRefs() throws Exception {\n \tpublic void testWritePack4SizeThinVsNoThin() throws Exception {\n \t\ttestWritePack4();\n \t\tfinal int sizePack4 = cos.getCount();\n+\t\ttearDown();\n \t\tsetUp();\n \t\ttestWritePack4ThinPack();\n \t\tfinal int sizePack4Thin = cos.getCount();\n-- \n1.6.0.3.640.g6331a\n"},{"id":"96799","messageId":"1228088435-23722-4-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-3-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 3/8] Turn off memory mapping in JGit unit tests by default","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:30Z","receivedAt":"2008-11-30T23:40:30Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"A system property named jgit.junit.usemmmap can be set to true to enable\nmemory mapping during unit testing.\n\nThe protected method configure can be overridden to do things\nlike configuring the JGit engine.\n\nSigned-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 30 ++++++++++++++++++++\n 1 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --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\nindex e164faf..3b08fa5 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -49,6 +49,19 @@\n import junit.framework.TestCase;\n import org.spearce.jgit.util.JGitTestUtil;\n \n+/**\n+ * Base class for most JGit unit tests.\n+ *\n+ * Sets up a predefined test repository and has support for creating additional\n+ * repositories and destroying them when the tests are finished.\n+ *\n+ * A system property jgit.junit.usemmmap defines whether memory mapping\n+ * is used. Memory mapping has an effect on the file system, in that memory\n+ * mapped files in java cannot be deleted as long as they mapped arrays have not\n+ * been reclaimed by the garbage collector. The programmer cannot control this\n+ * with precision, though hinting using {@link java.lang.System#gc}\n+ * often helps.\n+ */\n public abstract class RepositoryTestCase extends TestCase {\n \n \tprotected final File trashParent = new File(\"trash\");\n@@ -66,6 +79,22 @@\n \t\tjcommitter = new PersonIdent(\"J. Committer\", \"jcommitter@example.com\");\n \t}\n \n+\tprotected boolean packedGitMMAP;\n+\n+\t/**\n+\t * Configure JGit before setting up test repositories.\n+\t */\n+\tprotected void configure() {\n+\t\tpackedGitMMAP = \"true\".equals(System.getProperty(\"jgit.junit.usemmmap\"));\n+\t\tWindowCache.reconfigure(128*1024, 8192, packedGitMMAP, 8192);\n+\t}\n+\n+\t/**\n+\t * Utility method to delete a directory recursively. It is\n+\t * also used internally.\n+\t *\n+\t * @param dir\n+\t */\n \tprotected static void recursiveDelete(final File dir) {\n \t\tfinal File[] ls = dir.listFiles();\n \t\tif (ls != null) {\n@@ -123,6 +152,7 @@ protected static void checkFile(File f, final String checkData)\n \n \tpublic void setUp() throws Exception {\n \t\tsuper.setUp();\n+\t\tconfigure();\n \t\trecursiveDelete(trashParent);\n \t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis());\n \t\ttrash_git = new File(trash, \".git\");\n-- \n1.6.0.3.640.g6331a\n"},{"id":"96798","messageId":"1228088435-23722-5-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-4-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 4/8] Add a counter to make sure the test repo name is unique","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:31Z","receivedAt":"2008-11-30T23:40:31Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"System.currentTimeMillis() does not have the granularity\nnecessary to guarantee uniqueness. We keep it to make sure we\nhave unique names between different runs, but add a counter to\nmake it unique within the execution of a test suite.\n\nSigned-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --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\nindex 3b08fa5..6ea9b45 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -150,11 +150,13 @@ protected static void checkFile(File f, final String checkData)\n \n \tprotected Repository db;\n \n+\tprivate static int testcount;\n+\n \tpublic void setUp() throws Exception {\n \t\tsuper.setUp();\n \t\tconfigure();\n \t\trecursiveDelete(trashParent);\n-\t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis());\n+\t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis()+\".\"+(testcount++));\n \t\ttrash_git = new File(trash, \".git\");\n \n \t\tRuntime.getRuntime().addShutdownHook(new Thread() {\n@@ -200,7 +202,7 @@ protected void tearDown() throws Exception {\n \t * @throws IOException\n \t */\n \tprotected Repository createNewEmptyRepo() throws IOException {\n-\t\tFile newTestRepo = new File(trashParent, \"new\"+System.currentTimeMillis()+\"/.git\");\n+\t\tFile newTestRepo = new File(trashParent, \"new\"+System.currentTimeMillis()+\".\"+(testcount++)+\"/.git\");\n \t\tassertFalse(newTestRepo.exists());\n \t\tfinal Repository newRepo = new Repository(newTestRepo);\n \t\tnewRepo.create();\n-- \n1.6.0.3.640.g6331a\n"},{"id":"96803","messageId":"1228088435-23722-6-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-5-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 5/8] Make the cleanup less verbose when it fails to delete temporary stuff.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:32Z","receivedAt":"2008-11-30T23:40:32Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Pass the test case name for easier tracking of the test case that\ncauses problems.\n\nSigned-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 36 ++++++++++++++++----\n 1 files changed, 29 insertions(+), 7 deletions(-)\n\ndiff --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\nindex 6ea9b45..8e23bc1 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -96,21 +96,42 @@ protected void configure() {\n \t * @param dir\n \t */\n \tprotected static void recursiveDelete(final File dir) {\n+\t\trecursiveDelete(dir, false, null);\n+\t}\n+\n+\tprotected static boolean recursiveDelete(final File dir, boolean silent,\n+\t\t\tfinal String name) {\n+\t\tif (!dir.exists())\n+\t\t\treturn silent;\n \t\tfinal File[] ls = dir.listFiles();\n \t\tif (ls != null) {\n \t\t\tfor (int k = 0; k < ls.length; k++) {\n \t\t\t\tfinal File e = ls[k];\n \t\t\t\tif (e.isDirectory()) {\n-\t\t\t\t\trecursiveDelete(e);\n+\t\t\t\t\tsilent = recursiveDelete(e, silent, name);\n \t\t\t\t} else {\n-\t\t\t\t\te.delete();\n+\t\t\t\t\tif (!e.delete()) {\n+\t\t\t\t\t\tif (!silent) {\n+\t\t\t\t\t\t\tString msg = \"Warning: Failed to delete \" + e;\n+\t\t\t\t\t\t\tif (name != null)\n+\t\t\t\t\t\t\t\tmsg += \" in \" + name;\n+\t\t\t\t\t\t\tSystem.out.println(msg);\n+\t\t\t\t\t\t}\n+\t\t\t\t\t\tsilent = true;\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tdir.delete();\n-\t\tif (dir.exists()) {\n-\t\t\tSystem.out.println(\"Warning: Failed to delete \" + dir);\n+\t\tif (!dir.delete()) {\n+\t\t\tif (!silent) {\n+\t\t\t\tString msg = \"Warning: Failed to delete \" + dir;\n+\t\t\t\tif (name != null)\n+\t\t\t\t\tmsg += \" in \" + name;\n+\t\t\t\tSystem.out.println(msg);\n+\t\t\t}\n+\t\t\tsilent = true;\n \t\t}\n+\t\treturn silent;\n \t}\n \n \tprotected static void copyFile(final File src, final File dst)\n@@ -155,14 +176,15 @@ protected static void checkFile(File f, final String checkData)\n \tpublic void setUp() throws Exception {\n \t\tsuper.setUp();\n \t\tconfigure();\n-\t\trecursiveDelete(trashParent);\n+\t\tfinal String name = getClass().getName() + \".\" + getName();\n+\t\trecursiveDelete(trashParent, true, name);\n \t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis()+\".\"+(testcount++));\n \t\ttrash_git = new File(trash, \".git\");\n \n \t\tRuntime.getRuntime().addShutdownHook(new Thread() {\n \t\t\t@Override\n \t\t\tpublic void run() {\n-\t\t\t\trecursiveDelete(trashParent);\n+\t\t\t\trecursiveDelete(trashParent, false, name);\n \t\t\t}\n \t\t});\n \n-- \n1.6.0.3.640.g6331a\n"},{"id":"96804","messageId":"1228088435-23722-7-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-6-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 6/8] Cleanup after each test.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:33Z","receivedAt":"2008-11-30T23:40:33Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Automatically clean up any repositories created by the test cases.\nCleanup is attempted at the end of each test, but if that fails\nShutdown hooks attempt to clean up when the JVM exits. If memmory\nmapping is enabled (disabled by default in unit tests), gc is\ninvoked to make it more likely that cleanup will occur successfully.\nThe drawback is that this is much slower, which is the reason we\ndisble memory mapping by default in unit tests.\n\nSigned-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 62 ++++++++++++++++----\n 1 files changed, 50 insertions(+), 12 deletions(-)\n\ndiff --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\nindex 8e23bc1..aaa3592 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -45,6 +45,8 @@\n import java.io.InputStreamReader;\n import java.io.OutputStreamWriter;\n import java.io.Reader;\n+import java.util.ArrayList;\n+import java.util.List;\n \n import junit.framework.TestCase;\n import org.spearce.jgit.util.JGitTestUtil;\n@@ -95,8 +97,8 @@ protected void configure() {\n \t *\n \t * @param dir\n \t */\n-\tprotected static void recursiveDelete(final File dir) {\n-\t\trecursiveDelete(dir, false, null);\n+\tprotected void recursiveDelete(final File dir) {\n+\t\trecursiveDelete(dir, false, getClass().getName() + \".\" + getName());\n \t}\n \n \tprotected static boolean recursiveDelete(final File dir, boolean silent,\n@@ -170,9 +172,12 @@ protected static void checkFile(File f, final String checkData)\n \t}\n \n \tprotected Repository db;\n-\n+\tprivate static Thread shutdownhook;\n+\tprivate static List shutDownCleanups = new ArrayList();\n \tprivate static int testcount;\n \n+\tprivate ArrayList repositoriesToClose = new ArrayList();\n+\n \tpublic void setUp() throws Exception {\n \t\tsuper.setUp();\n \t\tconfigure();\n@@ -180,14 +185,23 @@ public void setUp() throws Exception {\n \t\trecursiveDelete(trashParent, true, name);\n \t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis()+\".\"+(testcount++));\n \t\ttrash_git = new File(trash, \".git\");\n-\n-\t\tRuntime.getRuntime().addShutdownHook(new Thread() {\n-\t\t\t@Override\n-\t\t\tpublic void run() {\n-\t\t\t\trecursiveDelete(trashParent, false, name);\n-\t\t\t}\n-\t\t});\n-\n+\t\tif (shutdownhook == null) {\n+\t\t\tshutdownhook = new Thread() {\n+\t\t\t\t@Override\n+\t\t\t\tpublic void run() {\n+\t\t\t\t\t// This may look superfluous, but is an extra attempt\n+\t\t\t\t\t// to clean up. First GC to release as many resources\n+\t\t\t\t\t// as possible and then try to clean up one test repo\n+\t\t\t\t\t// at a time (to record problems) and finally to drop\n+\t\t\t\t\t// the directory containing all test repositories.\n+\t\t\t\t\tSystem.gc();\n+\t\t\t\t\tfor (Runnable r : shutDownCleanups)\n+\t\t\t\t\t\tr.run();\n+\t\t\t\t\trecursiveDelete(trashParent, false, null);\n+\t\t\t\t}\n+\t\t\t};\n+\t\t\tRuntime.getRuntime().addShutdownHook(shutdownhook);\n+\t\t}\n \t\tdb = new Repository(trash_git);\n \t\tdb.create();\n \n@@ -213,6 +227,22 @@ copyFile(JGitTestUtil.getTestResourceFile(packs[k] + \".idx\"), new File(packDir,\n \n \tprotected void tearDown() throws Exception {\n \t\tdb.close();\n+\t\tfor (Repository r : repositoriesToClose)\n+\t\t\tr.close();\n+\n+\t\t// Since memory mapping is controlled by the GC we need to\n+\t\t// tell it this is a good time to clean up and unlock\n+\t\t// memory mapped files.\n+\t\tif (packedGitMMAP)\n+\t\t\tSystem.gc();\n+\n+\t\tfinal String name = getClass().getName() + \".\" + getName();\n+\t\trecursiveDelete(trash, false, name);\n+\t\tfor (Repository r : repositoriesToClose)\n+\t\t\trecursiveDelete(r.getWorkDir(), false, name);\n+\n+\t\trepositoriesToClose.clear();\n+\n \t\tsuper.tearDown();\n \t}\n \n@@ -224,10 +254,18 @@ protected void tearDown() throws Exception {\n \t * @throws IOException\n \t */\n \tprotected Repository createNewEmptyRepo() throws IOException {\n-\t\tFile newTestRepo = new File(trashParent, \"new\"+System.currentTimeMillis()+\".\"+(testcount++)+\"/.git\");\n+\t\tfinal File newTestRepo = new File(trashParent, \"new\"\n+\t\t\t\t+ System.currentTimeMillis() + \".\" + (testcount++) + \"/.git\");\n \t\tassertFalse(newTestRepo.exists());\n \t\tfinal Repository newRepo = new Repository(newTestRepo);\n \t\tnewRepo.create();\n+\t\tfinal String name = getClass().getName() + \".\" + getName();\n+\t\tshutDownCleanups.add(new Runnable() {\n+\t\t\tpublic void run() {\n+\t\t\t\trecursiveDelete(newTestRepo, false, name);\n+\t\t\t}\n+\t\t});\n+\t\trepositoriesToClose.add(newRepo);\n \t\treturn newRepo;\n \t}\n \n-- \n1.6.0.3.640.g6331a\n"},{"id":"96801","messageId":"1228088435-23722-8-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-7-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 7/8] Close files opened by unit testing framework","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:34Z","receivedAt":"2008-11-30T23:40:34Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Signed-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 12 ++++++++----\n .../tst/org/spearce/jgit/lib/T0007_Index.java | 10 +++++++---\n 2 files changed, 15 insertions(+), 7 deletions(-)\n\ndiff --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\nindex aaa3592..376a76e 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -165,10 +165,14 @@ protected File writeTrashFile(final String name, final String data)\n \tprotected static void checkFile(File f, final String checkData)\n \t\t\tthrows IOException {\n \t\tReader r = new InputStreamReader(new FileInputStream(f), \"ISO-8859-1\");\n-\t\tchar[] data = new char[(int) f.length()];\n-\t\tif (f.length() != r.read(data))\n-\t\t\tthrow new IOException(\"Internal error reading file data from \"+f);\n-\t\tassertEquals(checkData, new String(data));\n+\t\ttry {\n+\t\t\tchar[] data = new char[(int) f.length()];\n+\t\t\tif (f.length() != r.read(data))\n+\t\t\t\tthrow new IOException(\"Internal error reading file data from \"+f);\n+\t\t\tassertEquals(checkData, new String(data));\n+\t\t} finally {\n+\t\t\tr.close();\n+\t\t}\n \t}\n \n \tprotected Repository db;\ndiff --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\nindex 69f3a48..499812e 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0007_Index.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0007_Index.java\n@@ -424,9 +424,13 @@ public void test031_executeBit_coreModeFalse() throws IllegalArgumentException, \n \tprivate String content(File f) throws IOException {\n \t\tbyte[] buf = new byte[(int) f.length()];\n \t\tFileInputStream is = new FileInputStream(f);\n-\t\tint read = is.read(buf);\n-\t\tassertEquals(f.length(), read);\n-\t\treturn new String(buf, 0);\n+\t\ttry {\n+\t\t\tint read = is.read(buf);\n+\t\t\tassertEquals(f.length(), read);\n+\t\t\treturn new String(buf, 0);\n+\t\t} finally {\n+\t\t\tis.close();\n+\t\t}\n \t}\n \n \tprivate void delete(File f) throws IOException {\n-- \n1.6.0.3.640.g6331a\n"},{"id":"96800","messageId":"1228088435-23722-9-git-send-email-robin.rosenberg@dewire.com","threadId":"16509","inReplyTo":"1228088435-23722-8-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 8/8] Hard failure on unit test cleanups if they fail.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-30T23:40:35Z","receivedAt":"2008-11-30T23:40:35Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"This only has an effect on Windows that locks open files, but\nis a nice test that we actually clean up.\n\nSigned-off-by: Robin Rosenberg \n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java | 52 ++++++++++++--------\n 1 files changed, 32 insertions(+), 20 deletions(-)\n\ndiff --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\nindex 376a76e..22bf395 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java\n@@ -49,6 +49,7 @@\n import java.util.List;\n \n import junit.framework.TestCase;\n+\n import org.spearce.jgit.util.JGitTestUtil;\n \n /**\n@@ -93,16 +94,18 @@ protected void configure() {\n \n \t/**\n \t * Utility method to delete a directory recursively. It is\n-\t * also used internally.\n+\t * also used internally. If a file or directory cannote be removed\n+\t * it throws an AssertionFailure.\n \t *\n \t * @param dir\n \t */\n \tprotected void recursiveDelete(final File dir) {\n-\t\trecursiveDelete(dir, false, getClass().getName() + \".\" + getName());\n+\t\trecursiveDelete(dir, false, getClass().getName() + \".\" + getName(), true);\n \t}\n \n \tprotected static boolean recursiveDelete(final File dir, boolean silent,\n-\t\t\tfinal String name) {\n+\t\t\tfinal String name, boolean failOnError) {\n+\t\tassert !(silent && failOnError);\n \t\tif (!dir.exists())\n \t\t\treturn silent;\n \t\tfinal File[] ls = dir.listFiles();\n@@ -110,32 +113,42 @@ protected static boolean recursiveDelete(final File dir, boolean silent,\n \t\t\tfor (int k = 0; k < ls.length; k++) {\n \t\t\t\tfinal File e = ls[k];\n \t\t\t\tif (e.isDirectory()) {\n-\t\t\t\t\tsilent = recursiveDelete(e, silent, name);\n+\t\t\t\t\tsilent = recursiveDelete(e, silent, name, failOnError);\n \t\t\t\t} else {\n \t\t\t\t\tif (!e.delete()) {\n \t\t\t\t\t\tif (!silent) {\n-\t\t\t\t\t\t\tString msg = \"Warning: Failed to delete \" + e;\n-\t\t\t\t\t\t\tif (name != null)\n-\t\t\t\t\t\t\t\tmsg += \" in \" + name;\n-\t\t\t\t\t\t\tSystem.out.println(msg);\n+\t\t\t\t\t\t\treportDeleteFailure(name, failOnError, e);\n \t\t\t\t\t\t}\n-\t\t\t\t\t\tsilent = true;\n+\t\t\t\t\t\tsilent = !failOnError;\n \t\t\t\t\t}\n \t\t\t\t}\n \t\t\t}\n \t\t}\n \t\tif (!dir.delete()) {\n \t\t\tif (!silent) {\n-\t\t\t\tString msg = \"Warning: Failed to delete \" + dir;\n-\t\t\t\tif (name != null)\n-\t\t\t\t\tmsg += \" in \" + name;\n-\t\t\t\tSystem.out.println(msg);\n+\t\t\t\treportDeleteFailure(name, failOnError, dir);\n \t\t\t}\n-\t\t\tsilent = true;\n+\t\t\tsilent = !failOnError;\n \t\t}\n \t\treturn silent;\n \t}\n \n+\tprivate static void reportDeleteFailure(final String name,\n+\t\t\tboolean failOnError, final File e) {\n+\t\tString severity;\n+\t\tif (failOnError)\n+\t\t\tseverity = \"Error\";\n+\t\telse\n+\t\t\tseverity = \"Warning\";\n+\t\tString msg = severity + \": Failed to delete \" + e;\n+\t\tif (name != null)\n+\t\t\tmsg += \" in \" + name;\n+\t\tif (failOnError)\n+\t\t\tfail(msg);\n+\t\telse\n+\t\t\tSystem.out.println(msg);\n+\t}\n+\n \tprotected static void copyFile(final File src, final File dst)\n \t\t\tthrows IOException {\n \t\tfinal FileInputStream fis = new FileInputStream(src);\n@@ -186,7 +199,7 @@ public void setUp() throws Exception {\n \t\tsuper.setUp();\n \t\tconfigure();\n \t\tfinal String name = getClass().getName() + \".\" + getName();\n-\t\trecursiveDelete(trashParent, true, name);\n+\t\trecursiveDelete(trashParent, true, name, false); // Cleanup old failed stuff\n \t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis()+\".\"+(testcount++));\n \t\ttrash_git = new File(trash, \".git\");\n \t\tif (shutdownhook == null) {\n@@ -201,7 +214,7 @@ public void run() {\n \t\t\t\t\tSystem.gc();\n \t\t\t\t\tfor (Runnable r : shutDownCleanups)\n \t\t\t\t\t\tr.run();\n-\t\t\t\t\trecursiveDelete(trashParent, false, null);\n+\t\t\t\t\trecursiveDelete(trashParent, false, null, false);\n \t\t\t\t}\n \t\t\t};\n \t\t\tRuntime.getRuntime().addShutdownHook(shutdownhook);\n@@ -241,10 +254,9 @@ protected void tearDown() throws Exception {\n \t\t\tSystem.gc();\n \n \t\tfinal String name = getClass().getName() + \".\" + getName();\n-\t\trecursiveDelete(trash, false, name);\n+\t\trecursiveDelete(trash, false, name, true);\n \t\tfor (Repository r : repositoriesToClose)\n-\t\t\trecursiveDelete(r.getWorkDir(), false, name);\n-\n+\t\t\trecursiveDelete(r.getWorkDir(), false, name, true);\n \t\trepositoriesToClose.clear();\n \n \t\tsuper.tearDown();\n@@ -266,7 +278,7 @@ protected Repository createNewEmptyRepo() throws IOException {\n \t\tfinal String name = getClass().getName() + \".\" + getName();\n \t\tshutDownCleanups.add(new Runnable() {\n \t\t\tpublic void run() {\n-\t\t\t\trecursiveDelete(newTestRepo, false, name);\n+\t\t\t\trecursiveDelete(newTestRepo, false, name, false);\n \t\t\t}\n \t\t});\n \t\trepositoriesToClose.add(newRepo);\n-- \n1.6.0.3.640.g6331a\n"},{"id":"96891","messageId":"alpine.DEB.1.00.0812020016490.18880@intel-tinevez-2-302","threadId":"16509","inReplyTo":"200811291301.12095.robin.rosenberg@dewire.com","subject":"Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-01T23:18:42Z","receivedAt":"2008-12-01T23:18:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 29 Nov 2008, Robin Rosenberg wrote:\n\n> [Repository refactoring] Would be cool, but having that diff engine is \n> more important to me.\n\nStay tuned. I have something that outputs something resembling a diff \nnow. Of course, the output is not correct yet, due to bugs I introduced \ncunnily when trying to fix another bug.\n\nI'll keep you posted,\nDscho\n"},{"id":"96964","messageId":"20081202163856.GR23984@spearce.org","threadId":"16509","inReplyTo":"1228088435-23722-1-git-send-email-robin.rosenberg@dewire.com","subject":"Re: [JGIT PATCH v2 0/8] Unit test cleanups","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-02T16:38:56Z","receivedAt":"2008-12-02T16:38:56Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg wrote:\n> A completele reworked set of patches, including fixing a couple\n> more forgot-to-close bugs and Shawns suggestion that we disable\n> memory mapping in junit tests by default.\n...\n> Robin Rosenberg (8):\n> Drop unneeded code in unit tests\n> Cleanup malformed test cases\n> Turn off memory mapping in JGit unit tests by default\n> Add a counter to make sure the test repo name is unique\n> Make the cleanup less verbose when it fails to delete temporary\n> stuff.\n> Cleanup after each test.\n> Close files opened by unit testing framework\n> Hard failure on unit test cleanups if they fail.\n> \n> .../tst/org/spearce/jgit/lib/PackWriterTest.java | 3 +\n> .../org/spearce/jgit/lib/RepositoryTestCase.java | 152 +++++++++++++++++---\n> .../tst/org/spearce/jgit/lib/T0007_Index.java | 10 +-\n> 3 files changed, 139 insertions(+), 26 deletions(-)\n\nMerged. The series looked really good to me. Second time is the\ncharm, eh? :-)\n\n-- \nShawn.\n"}]}