Re: [PATCH 0/3] t0000 cleanups
Jeff King wrote: On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote: These scratch areas for sub-tests should be under the t trash directory, but because the TEST_OUTPUT_DIRECTORY setting from the toplevel test leaks [...] This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that as a default if it is not explicitly set. So I should have said something like the following instead: These scratch areas for sub-tests should be under the t trash directory, but because TEST_OUTPUT_DIRECTORY defaults to TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh, the sub-test trash directories are created under the toplevel t/ directory instead. Because some of the sub-tests simulate failures, their trash directories are kept around. Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately for sub-tests. Thanks for catching it. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jonathan Nieder jrnie...@gmail.com writes: Jeff King wrote: On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote: These scratch areas for sub-tests should be under the t trash directory, but because the TEST_OUTPUT_DIRECTORY setting from the toplevel test leaks [...] This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that as a default if it is not explicitly set. So I should have said something like the following instead: These scratch areas for sub-tests should be under the t trash directory, but because TEST_OUTPUT_DIRECTORY defaults to TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh, the sub-test trash directories are created under the toplevel t/ directory instead. Because some of the sub-tests simulate failures, their trash directories are kept around. I had a private rewrite queued already, but the above is easier to read, so I'll replace it with this. Thanks. Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately for sub-tests. Thanks for catching it. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote: I think it can be better, since the commit message left me scratching my head while the patch itself seems pretty simple. How about something like the following? I am fine with that format, though... Analysis and fix: These scratch areas for sub-tests should be under the t trash directory, but because the TEST_OUTPUT_DIRECTORY setting from the toplevel test leaks into the environment they are created under the toplevel output directory (typically t/) instead. Because some of the sub-tests simulate failures, their trash directories are kept around. This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that as a default if it is not explicitly set. The rest of your rewrite looks correct. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jonathan Nieder jrnie...@gmail.com writes: Jeff King wrote: When I want to debug a failing test, I often end up doing: cd t ./t4107-tab -v -i cd tratab The test names are long, so tab-completing on the trash directory is very helpful. Lately I've noticed that there are a bunch of crufty trash directories in my t/ directory, which makes my tab-completion more annoying. Ah, and if I'd read this then I wouldn't have had to be confused at all. Would it work to replace the commit message with something like this? The third paragraph of 1/3 sufficiently covers it, no? We could add It makes it less convenient to use tab completion 'cd t/traTAB' to go to the trash directory of the failed test to inspect the situation after ... left in the t/ directory., though. Once upon a time, the test-lib library would create trash directories in the current working directory, unless we were explicitly told to put it elsewhere via --root. As a result, t created the sub-test trash directories inside its own trash directory. However, we noticed that this did not cover all cases, since we would need to respect $TEST_OUTPUT_DIRECTORY even if --root is not given (or is relative). Commit 38b074d fixed this to consistently use the full path. As a result, trash directories used by t's sub-tests are now created in git's original test output directory rather than in our trash directory. Furthermore, since some of the sub-tests simulate failures, the trash directories do not get cleaned up, and the cruft is left in the t/ directory. We could fix this by passing a new --root=$TRASH_DIRECTORY option to the sub-test. However, we do not want the sub-tests to write anything at all to git's directory (e.g., they should not be writing to t/test-results, either, although this is already handled by separate code). So the best solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely in the sub-test, which covers this case, as well as any future ones. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Jeff King wrote: When I want to debug a failing test, I often end up doing: cd t ./t4107-tab -v -i cd tratab The test names are long, so tab-completing on the trash directory is very helpful. Lately I've noticed that there are a bunch of crufty trash directories in my t/ directory, which makes my tab-completion more annoying. Ah, and if I'd read this then I wouldn't have had to be confused at all. [...] The third paragraph of 1/3 sufficiently covers it, no? We could add It makes it less convenient to use tab completion 'cd t/traTAB' to go to the trash directory of the failed test to inspect the situation after ... left in the t/ directory., though. [4 paragraphs snipped] I think it can be better, since the commit message left me scratching my head while the patch itself seems pretty simple. How about something like the following? First, describing the problem: Running t produces more trash directories than expected and does not clean up after itself: $ ./t-basic.sh [...] $ ls -d trash\ directory.* trash directory.failing-cleanup trash directory.mixed-results1 trash directory.mixed-results2 trash directory.partial-pass trash directory.test-verbose trash directory.test-verbose-only-2 Analysis and fix: These scratch areas for sub-tests should be under the t trash directory, but because the TEST_OUTPUT_DIRECTORY setting from the toplevel test leaks into the environment they are created under the toplevel output directory (typically t/) instead. Because some of the sub-tests simulate failures, their trash directories are kept around. Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately for sub-tests. And then, optionally, describing rejected alternatives: An alternative fix would be to pass the --root parameter that only specifies where to put the trash directories, which would also work. However, using TEST_OUTPUT_DIRECTORY is more futureproof in case tests want to write more output in addition to the test-results/ (which are already suppressed in sub-tests using the HARNESS_ACTIVE setting) and trash directories. And more analysis of why this wasn't caught in the first place: This fixes a regression introduced by 38b074d (t/test-lib.sh: fix TRASH_DIRECTORY handling, 2013-04-14). Before then, the TEST_OUTPUT_DIRECTORY setting was not respected consistently so most tests did their work in a trash subdirectory of the current directory instead of the output dir. Does that make sense? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jeff King wrote: When I want to debug a failing test, I often end up doing: cd t ./t4107-tab -v -i cd tratab The test names are long, so tab-completing on the trash directory is very helpful. Lately I've noticed that there are a bunch of crufty trash directories in my t/ directory, which makes my tab-completion more annoying. Ah, and if I'd read this then I wouldn't have had to be confused at all. Would it work to replace the commit message with something like this? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
