There is some risk with your patch: if two tests try to create the same dir at the same time ... with the patch, one of the tests will create the dir, and the 2nd test will see it already exists and then use that directory illegally (ie, share it with the first test). That will lead to very confusing test failures (if it happens).
Ie, the idea of this API is atomicity: the file/dir did not exist before, and this caller succeeded in creating it, it returns true, meaning 1) the file/dir is newly created, and 2) is "private" to you. Mike McCandless http://blog.mikemccandless.com On Wed, Dec 19, 2012 at 1:57 PM, Shai Erera <ser...@gmail.com> wrote: > If nobody objects, I'd like to commit this simple fix. Currently, if a test > fails, I cannot re-run it w/ same seed unless I delete the directory > manually, or in the test itself (since CloseableFile doesn't delete the dir > if the test failed). > > Shai > > > On Wed, Dec 19, 2012 at 5:15 PM, Shai Erera <ser...@gmail.com> wrote: >>> >>> I patched _TestUtil like so, and the test passes >> >> >> Sorry, I meant to say that if fails where I expect it to fail, not on >> Access is Denied. >> >> Shai >> >> >> On Wed, Dec 19, 2012 at 5:14 PM, Shai Erera <ser...@gmail.com> wrote: >>> >>> Hi >>> >>> When a test fails, the file system directories that it created are not >>> deleted. >>> >>> At least on Windows, when you run the test with the same seed again, >>> _TestUtil.getTempDir fails on "Access Denied". The reason is that it calls >>> file.createNewFile() on a directory. I don't know if it's specific to >>> Windows, but I tried this test with both J9 and Oracle JVMs: >>> >>> @Test >>> public void testAccessDenied() throws Exception { >>> _TestUtil.getTempDir("testAccessDenied").mkdirs(); >>> fail("msg"); >>> } >>> >>> Because of the fail() in the end, the directory is not deleted. If you >>> run it with the same seed twice (say, -Dtests.seed=6EDF27F12DB68F8D), then >>> you'll get: >>> >>> java.lang.RuntimeException: java.io.IOException: Access is denied >>> at >>> __randomizedtesting.SeedInfo.seed([6EDF27F12DB68F8D:668298B50F63FCD2]:0) >>> at org.apache.lucene.util._TestUtil.getTempDir(_TestUtil.java:107) >>> at >>> org.apache.lucene.TestAssertions.testAccessDenied(TestAssertions.java:62) >>> >>> I patched _TestUtil like so, and the test passes: >>> >>> Index: >>> lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java >>> =================================================================== >>> --- lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java >>> (revision 1423868) >>> +++ lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java >>> (working copy) >>> @@ -750,7 +750,7 @@ >>> final Random random = new >>> Random(RandomizedContext.current().getRandom().nextLong()); >>> do { >>> result = genTempFile(random, prefix, newSuffix, directory); >>> - } while (!result.createNewFile()); >>> + } while (result.exists() || !result.createNewFile()); >>> return result; >>> } >>> >>> Basically, adding an exists() check before createNewFile(). Even though >>> createNewFile() documents that it returns false if the file exists, it does >>> not specify the behavior when the file exists and is a directory, and >>> obviously fails. >>> >>> I made sure that calling genTempFile like so in a loop won't ruin >>> randomness - since it only uses random.nextInt() once, I think it's safe to >>> add this check? >>> >>> Shai >> >> > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
