Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
Hi, Jeff King wrote: 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. So the idea if I am reading correctly is Instead of relying on the implicit output directory chosen with chdir, which doesn't even work any more, set TEST_OUTPUT_DIRECTORY to decide where output for the sub-tests used by t's sanity checks for the test harness go. I'm not sure I completely understand the regression caused by 38b074d. Is the idea that before that commit, TEST_OUTPUT_DIRECTORY was only used for the test-results/ directory so the only harm done was some mixing of test results? What is the symptom this patch alleviates? As a result, t's sub-tests are now created in git's original test output directory rather than in our trash directory. This might be the source of my confusion. Is sub-tests an abbreviation for sub-test trash directories here? 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). Ah, HARNESS_ACTIVE prevents output of test-results. Does the git test harness write something else to TEST_OUTPUT_DIRECTORY? Is the idea that using --root would be functionally equivalent but (1) more confusing and (2) less futureproof? 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. So, to sum up: if I understand correctly - git used to only use TEST_OUTPUT_DIRECTORY to decide where test results go. You'd have to use --root to set a custom location for trash directories. - in that old setup, t leaves around extra trash directories with --root, since the sub-tests inherit the parent test's $root and put trash directories there. - after 38b074d, that old problem still exists and furthermore t leaves around extra trash directories even when --root is not in use, since the sub-tests inherit the value of TEST_OUTPUT_DIRECTORY from the parent test. - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root problem) by setting TEST_OUTPUT_DIRECTORY explicitly Does that sound right? If so, should sub-tests unset $root, too? Thanks and hope that helps, 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 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
Jonathan Nieder wrote: - git used to only use TEST_OUTPUT_DIRECTORY to decide where test results go. You'd have to use --root to set a custom location for trash directories. - in that old setup, t leaves around extra trash directories with --root, since the sub-tests inherit the parent test's $root and put trash directories there. Nope, since sub-tests are run with fork + exec, which loses $root... - after 38b074d, that old problem still exists and furthermore t leaves around extra trash directories even when --root is not in use, since the sub-tests inherit the value of TEST_OUTPUT_DIRECTORY from the parent test. ... meaning the TEST_OUTPUT_DIRECTORY problem is the only problem - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root problem) by setting TEST_OUTPUT_DIRECTORY explicitly Does that sound right? If so, should sub-tests unset $root, too? ... and there's no need to 'unset root'. So the patch itself looks right. I think describing the symptoms up front would probably be enough to make the commit message less confusing to read. 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 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
On Sat, Dec 28, 2013 at 02:13:13PM -0800, Jonathan Nieder wrote: So the idea if I am reading correctly is Instead of relying on the implicit output directory chosen with chdir, which doesn't even work any more, set TEST_OUTPUT_DIRECTORY to decide where output for the sub-tests used by t's sanity checks for the test harness go. Right. I'm not sure I completely understand the regression caused by 38b074d. Is the idea that before that commit, TEST_OUTPUT_DIRECTORY was only used for the test-results/ directory so the only harm done was some mixing of test results? $TEST_OUTPUT_DIRECTORY was actually used in $TRASH_DIRECTORY, but some code paths properly used $TRASH_DIRECTORY, and some used another variable that (sometimes) contained a relative form of $TRASH_DIRECTORY. The creation of the repo was one such code-path. So there were already potentially problems before 38b074d (any sub-test looking at its $TRASH_DIRECTORY variable would find the wrong path), but I do not know offhand if it could trigger any bugs. Post-38b074d, we consistently use $TRASH_DIRECTORY (and therefore respect $TEST_OUTPUT_DIRECTORY) everywhere. What is the symptom this patch alleviates? As a result, t's sub-tests are now created in git's original test output directory rather than in our trash directory. This might be the source of my confusion. Is sub-tests an abbreviation for sub-test trash directories here? Yes, I should have said sub-test trash directories. And I think that answers your what is the symptom question. 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). Ah, HARNESS_ACTIVE prevents output of test-results. Yes. My original notion was Oh, and this fixes broken test-results, too!. But then I noticed that it is already handled in a different way. :) Does the git test harness write something else to TEST_OUTPUT_DIRECTORY? Is the idea that using --root would be functionally equivalent but (1) more confusing and (2) less futureproof? Exactly. I do not think TEST_OUTPUT_DIRECTORY is used for anything else, but if someone were to ever add a new use, the sub-tests would almost certainly want that use to affect only the t trash directory. So, to sum up: if I understand correctly You answered these yourself in your follow-up. :) So the patch itself looks right. I think describing the symptoms up front would probably be enough to make the commit message less confusing to read. Would adding the missing trash directories wording above be sufficient? -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
