{"thread":{"id":"16510","subject":"[JGIT PATCH 1/4] Make the cleanup less verbose when it fails to delete temporary stuff.","startedAt":"2008-11-27T21:15:31Z","lastAt":"2008-11-27T21:15:35Z","messageCount":5,"participants":["Robin Rosenberg"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"96642","messageId":"1227820535-9785-1-git-send-email-robin.rosenberg@dewire.com","threadId":"16510","inReplyTo":null,"subject":"[JGIT PATCH 0/4] RepositoryTestCase cleanups","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-27T21:15:31Z","receivedAt":"2008-11-27T21:15:31Z","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":"96641","messageId":"1227820535-9785-2-git-send-email-robin.rosenberg@dewire.com","threadId":"16510","inReplyTo":"1227820535-9785-1-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 1/4] Make the cleanup less verbose when it fails to delete temporary stuff.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-27T21:15:32Z","receivedAt":"2008-11-27T21:15: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 <robin.rosenberg@dewire.com>\n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java   |   59 ++++++++++++++-----\n 1 files changed, 43 insertions(+), 16 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..9c272f6 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@@ -66,22 +66,43 @@\n \t\tjcommitter = new PersonIdent(\"J. Committer\", \"jcommitter@example.com\");\n \t}\n \n-\tprotected static void recursiveDelete(final File dir) {\n+\tprotected static boolean recursiveDelete(final File dir) {\n+\t\treturn recursiveDelete(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@@ -121,19 +142,25 @@ protected static void checkFile(File f, final String checkData)\n \n \tprotected Repository db;\n \n+\tprivate static int testcount;\n+\tprivate static Thread shutdownhook;\n+\n \tpublic void setUp() throws Exception {\n \t\tsuper.setUp();\n-\t\trecursiveDelete(trashParent);\n-\t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis());\n+\t\tSystem.gc();\n+\t\ttrash = new File(trashParent,\"trash\"+System.currentTimeMillis()+\".\"+(testcount++));\n+\t\tfinal String name = getClass().getName() + \".\" + getName();\n+\t\trecursiveDelete(trashParent, true, name);\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}\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\trecursiveDelete(trashParent, false, name);\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-- \n1.6.0.3.640.g6331a\n"},{"id":"96645","messageId":"1227820535-9785-3-git-send-email-robin.rosenberg@dewire.com","threadId":"16510","inReplyTo":"1227820535-9785-2-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 2/4] Add shutdown hooks to try to clean up after unit tests anyway","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-27T21:15:33Z","receivedAt":"2008-11-27T21:15:33Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java   |   18 +++++++++++++++---\n 1 files changed, 15 insertions(+), 3 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 9c272f6..ef4fd1b 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@@ -66,8 +66,8 @@\n \t\tjcommitter = new PersonIdent(\"J. Committer\", \"jcommitter@example.com\");\n \t}\n \n-\tprotected static boolean recursiveDelete(final File dir) {\n-\t\treturn recursiveDelete(dir, false, null);\n+\tprotected boolean recursiveDelete(final File dir) {\n+\t\treturn recursiveDelete(dir, false, getClass().getName() + \".\" + getName());\n \t}\n \n \tprotected static boolean recursiveDelete(final File dir, boolean silent,\n@@ -161,6 +161,12 @@ public void run() {\n \t\t\t};\n \t\t\tRuntime.getRuntime().addShutdownHook(shutdownhook);\n \t\t}\n+\t\tRuntime.getRuntime().addShutdownHook(new Thread() {\n+\t\t\t@Override\n+\t\t\tpublic void run() {\n+\t\t\t\trecursiveDelete(trash);\n+\t\t\t}\n+\t\t});\n \t\tdb = new Repository(trash_git);\n \t\tdb.create();\n \n@@ -197,12 +203,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()+\"/.git\");\n+\t\tfinal File 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\tRuntime.getRuntime().addShutdownHook(new Thread() {\n+\t\t\t@Override\n+\t\t\tpublic void run() {\n+\t\t\t\trecursiveDelete(newTestRepo);\n+\t\t\t}\n+\t\t});\n \t\treturn newRepo;\n \t}\n \n-- \n1.6.0.3.640.g6331a\n"},{"id":"96643","messageId":"1227820535-9785-4-git-send-email-robin.rosenberg@dewire.com","threadId":"16510","inReplyTo":"1227820535-9785-3-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 3/4] Cleanup malformed test cases","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-27T21:15:34Z","receivedAt":"2008-11-27T21:15:34Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"---\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":"96644","messageId":"1227820535-9785-5-git-send-email-robin.rosenberg@dewire.com","threadId":"16510","inReplyTo":"1227820535-9785-4-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 4/4] Automatically clean up any repositories created by the test cases","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-11-27T21:15:35Z","receivedAt":"2008-11-27T21:15:35Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n---\n .../org/spearce/jgit/lib/RepositoryTestCase.java   |   21 +++++++++++++++++++-\n 1 files changed, 20 insertions(+), 1 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 ef4fd1b..cab65a0 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,7 @@\n import java.io.InputStreamReader;\n import java.io.OutputStreamWriter;\n import java.io.Reader;\n+import java.util.ArrayList;\n \n import junit.framework.TestCase;\n import org.spearce.jgit.util.JGitTestUtil;\n@@ -145,6 +146,8 @@ protected static void checkFile(File f, final String checkData)\n \tprivate static int testcount;\n \tprivate static Thread shutdownhook;\n \n+\tprivate ArrayList<Repository> repositoriesToClose = new ArrayList<Repository>();\n+\n \tpublic void setUp() throws Exception {\n \t\tsuper.setUp();\n \t\tSystem.gc();\n@@ -192,6 +195,20 @@ 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+\t\t}\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// mmemory mapped files.\n+\t\tSystem.gc();\n+\n+\t\trecursiveDelete(trash, false, getName());\n+\t\tfor (Repository r : repositoriesToClose) {\n+\t\t\trecursiveDelete(r.getWorkDir(), false, getName());\n+\t\t}\n+\t\trepositoriesToClose.clear();\n+\n \t\tsuper.tearDown();\n \t}\n \n@@ -209,12 +226,14 @@ protected Repository createNewEmptyRepo() throws IOException {\n \t\tassertTrue(unusedDir.mkdirs());\n \t\tfinal Repository newRepo = new Repository(newTestRepo);\n \t\tnewRepo.create();\n+\t\tfinal String name = getClass().getName() + \".\" + getName();\n \t\tRuntime.getRuntime().addShutdownHook(new Thread() {\n \t\t\t@Override\n \t\t\tpublic void run() {\n-\t\t\t\trecursiveDelete(newTestRepo);\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"}]}