threads / patch / 35579

patch, 3 partst0000 cleanups

Subject: [PATCH 0/3] t0000 cleanups

## tl;dr

18 messages between Dec 28, 2013 and Jan 3, 2014. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Jeff King· Dec 28, 2013, 09:27 UTC · lore
When I want to debug a failing test, I often end up doing:
  cd t
  ./t4107-<tab> -v -i
  cd tra<tab>

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.

It turns out they're leftovers from t0000 running, due to a bad interaction with some other fixes from last April. The first patch fixes that.

The second patch is a follow-on cleanup enabled by the first.

The third is an unrelated cleanup that I noticed when I ran t0000 47 times in a row. :)

  [1/3]: t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
  [2/3]: t0000: simplify HARNESS_ACTIVE hack
  [3/3]: t0000: drop "known breakage" test
 t/t0000-basic.sh | 19 +++++++------------
 t/test-lib.sh    |  2 --
 2 files changed, 7 insertions(+), 14 deletions(-)
-Peff
Jeff King· Dec 28, 2013, 09:29 UTC · re: Jeff King · lore

[PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

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, t0000 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, t0000'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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0000-basic.sh | 2 ++
 1 file changed, 2 insertions(+)
Show changes to t/t0000-basic.sh +2 −0
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 10be52b..bc4e3e2 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -71,6 +71,8 @@ run_sub_test_lib_test () {
 		cat >>"$name.sh" &&
 		chmod +x "$name.sh" &&
 		export TEST_DIRECTORY &&
+		TEST_OUTPUT_DIRECTORY=$(pwd) &&
+		export TEST_OUTPUT_DIRECTORY &&
 		./"$name.sh" "$@" >out 2>err
 	)
 }

-- 
1.8.5.1.399.g900e7cd
Jonathan Nieder· Dec 28, 2013, 22:13 UTC · re: Jeff King · lore

Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

Hi,
Jeff King wrote:
Show 10 quoted lines
> 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,
> t0000 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 t0000'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, t0000'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?

Show 9 quoted lines
>            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, t0000 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
   t0000 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

Jonathan Nieder· Dec 28, 2013, 22:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

Jonathan Nieder wrote:
Show 7 quoted lines
>  - 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, t0000 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
>    t0000 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
Jeff King· Dec 29, 2013, 07:17 UTC · re: Jonathan Nieder · lore

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 t0000'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.

Show 8 quoted lines
> What is the symptom this patch alleviates?
> 
> > As a result, t0000'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.

Show 7 quoted lines
> > 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 t0000 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
Jeff King· Dec 28, 2013, 09:31 UTC · re: Jeff King · lore

[PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack

Commit 517cd55 set HARNESS_ACTIVE unconditionally in sub-tests, because that value affects the output of "--verbose". t0000 needs stable output from its sub-tests, and we may or may not be running under a TAP harness.

That commit made the decision to always set the variable, since it has another useful side effect, which is suppressing writes to t/test-results by the sub-tests (which would just pollute the real results).

Since the last commit, though, the sub-tests have their own test-results directories, so this is no longer an issue. We can now update a few comments that are no longer accurate nor necessary.

We can also revisit the choice of HARNESS_ACTIVE. Since we must choose one value for stability, it's probably saner to have it off. This means that future patches could test things like the test-results writing, or the "--quiet" option, which is currently ignored when run under a harness.

Signed-off-by: Jeff King <peff@peff.net>
---
I do not have any plans to write such tests, and I'd be OK if we wanted
to stop this just at fixing up the comments. But it took me a while to
figure out what is going on, and I believe unsetting HARNESS_ACTIVE in
the sub-tests is the choice that is least likely to cause somebody in
the future to have to re-figure it out. :)
 t/t0000-basic.sh | 14 +++++---------
 t/test-lib.sh    |  2 --
 2 files changed, 5 insertions(+), 11 deletions(-)
Show changes to 2 files +5 −11

t/t0000-basic.sh, t/test-lib.sh

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index bc4e3e2..e6c5b63 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -50,11 +50,11 @@ run_sub_test_lib_test () {
 	shift 2
 	mkdir "$name" &&
 	(
-		# Pretend we're a test harness.  This prevents
-		# test-lib from writing the counts to a file that will
-		# later be summarized, showing spurious "failed" tests
-		HARNESS_ACTIVE=t &&
-		export HARNESS_ACTIVE &&
+		# Pretend we're not running under a test harness, whether we
+		# are or not. The test-lib output depends on the setting of
+		# this variable, so we need a stable setting under which to run
+		# the sub-test.
+		sane_unset HARNESS_ACTIVE &&
 		cd "$name" &&
 		cat >"$name.sh" <<-EOF &&
 		#!$SHELL_PATH
@@ -235,16 +235,13 @@ test_expect_success 'test --verbose' '
 	grep -v "^Initialized empty" test-verbose/out+ >test-verbose/out &&
 	check_sub_test_lib_test test-verbose <<-\EOF
 	> expecting success: true
-	> Z
 	> ok 1 - passing test
 	> Z
 	> expecting success: echo foo
 	> foo
-	> Z
 	> ok 2 - test with output
 	> Z
 	> expecting success: false
-	> Z
 	> not ok 3 - failing test
 	> #	false
 	> Z
@@ -267,7 +264,6 @@ test_expect_success 'test --verbose-only' '
 	> Z
 	> expecting success: echo foo
 	> foo
-	> Z
 	> ok 2 - test with output
 	> Z
 	> not ok 3 - failing test
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1cf78d5..1531c24 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -481,8 +481,6 @@ test_at_end_hook_ () {
 test_done () {
 	GIT_EXIT_OK=t
 
-	# Note: t0000 relies on $HARNESS_ACTIVE disabling the .counts
-	# output file
 	if test -z "$HARNESS_ACTIVE"
 	then
 		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"

-- 
1.8.5.1.399.g900e7cd
Jonathan Nieder· Dec 28, 2013, 22:14 UTC · re: Jeff King · lore

Re: [PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack

Jeff King wrote:
Show 16 quoted lines
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -50,11 +50,11 @@ run_sub_test_lib_test () {
>  	shift 2
>  	mkdir "$name" &&
>  	(
> -		# Pretend we're a test harness.  This prevents
> -		# test-lib from writing the counts to a file that will
> -		# later be summarized, showing spurious "failed" tests
> -		HARNESS_ACTIVE=t &&
> -		export HARNESS_ACTIVE &&
> +		# Pretend we're not running under a test harness, whether we
> +		# are or not. The test-lib output depends on the setting of
> +		# this variable, so we need a stable setting under which to run
> +		# the sub-test.
> +		sane_unset HARNESS_ACTIVE &&
Makes sense.

Thanks, Jonathan

Jeff King· Dec 28, 2013, 09:33 UTC · re: Jeff King · lore

[PATCH 3/3] t0000: drop "known breakage" test

Having a simulated "known breakage" test means that the test suite will always tell us there is a bug to be fixed, even though it is only simulated.

The right way to test this is in a sub-test, that can also check that we provide the correct exit status and output. Fortunately, we already have such a test (added much later by 5ebf89e).

We could arguably get rid of the simulated success test immediately above, as well, as it is also redundant with the tests added in 5ebf89e. However, it does not have the annoying behavior of the "known breakage" test. It may also be easier to debug if the test suite is truly broken, since it is not a test-within-a-test, as the later tests are.

Signed-off-by: Jeff King <peff@peff.net>
---
I am not _that_ bothered by the "known breakage", but AFAICT there is
zero benefit to keeping this redundant test. But maybe I am missing
something.
 t/t0000-basic.sh | 3 ---
 1 file changed, 3 deletions(-)
Show changes to t/t0000-basic.sh +0 −3
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index e6c5b63..a2bb63c 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -41,9 +41,6 @@ test_expect_success '.git/objects should have 3 subdirectories' '
 test_expect_success 'success is reported like this' '
 	:
 '
-test_expect_failure 'pretend we have a known breakage' '
-	false
-'
 
 run_sub_test_lib_test () {
 	name="$1" descr="$2" # stdin is the body of the test code

-- 
1.8.5.1.399.g900e7cd
Jonathan Nieder· Dec 28, 2013, 20:51 UTC · re: Jeff King · lore

Re: [PATCH 3/3] t0000: drop "known breakage" test

Jeff King wrote:
> I am not _that_ bothered by the "known breakage", but AFAICT there is
> zero benefit to keeping this redundant test.

Devil's advocate: it ensures that anyone wrapping git's tests (like the old smoketest infrastructure experiment) is able to handle an expected failure.

But in practice I don't mind the behavior before or after this patch. If the test harness is that broken, we'll know. And people writing code that wraps git's tests can write their own custom sanity-checks.

Jeff King· Dec 29, 2013, 07:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH 3/3] t0000: drop "known breakage" test

On Sat, Dec 28, 2013 at 12:51:04PM -0800, Jonathan Nieder wrote:
Show 8 quoted lines
> Jeff King wrote:
> 
> > I am not _that_ bothered by the "known breakage", but AFAICT there is
> > zero benefit to keeping this redundant test.
> 
> Devil's advocate: it ensures that anyone wrapping git's tests (like
> the old smoketest infrastructure experiment) is able to handle an
> expected failure.

Thanks. One of the things I love about open source is that as soon as I say "I can't see how...", the answer is crowd-sourced for me. :)

That being said, even if the test has a non-zero possible value...
> But in practice I don't mind the behavior before or after this patch.
> If the test harness is that broken, we'll know.  And people writing
> code that wraps git's tests can write their own custom sanity-checks.

...I think for these reasons that the value is smaller than the disruption caused by the test, and the patch is a net win.

-Peff
Jonathan Nieder· Dec 28, 2013, 22:21 UTC · re: Jeff King · lore

Re: [PATCH 0/3] t0000 cleanups

Jeff King wrote:
Show 10 quoted lines
> When I want to debug a failing test, I often end up doing:
>
>   cd t
>   ./t4107-<tab> -v -i
>   cd tra<tab>
>
> 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

Junio C Hamano· Dec 30, 2013, 18:30 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/3] t0000 cleanups

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 16 quoted lines
> Jeff King wrote:
>
>> When I want to debug a failing test, I often end up doing:
>>
>>   cd t
>>   ./t4107-<tab> -v -i
>>   cd tra<tab>
>>
>> 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/tra<TAB>' 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,
    t0000 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 t0000'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.
Jonathan Nieder· Dec 30, 2013, 18:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] t0000 cleanups

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Jeff King wrote:
Show 13 quoted lines
>>> When I want to debug a failing test, I often end up doing:
>>>
>>>   cd t
>>>   ./t4107-<tab> -v -i
>>>   cd tra<tab>
>>>
>>> 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/tra<TAB>'
> 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 t0000 produces more trash directories than expected
	and does not clean up after itself:
	 $ ./t0000-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 t0000
	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

Jeff King· Dec 31, 2013, 10:33 UTC · re: Jonathan Nieder · lore

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...
Show 8 quoted lines
> Analysis and fix:
> 
> 	These scratch areas for sub-tests should be under the t0000
> 	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. t0000 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
Jonathan Nieder· Jan 2, 2014, 22:28 UTC · re: Jeff King · lore

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 t0000
>> 	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. t0000 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 t0000 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
Junio C Hamano· Jan 2, 2014, 22:41 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/3] t0000 cleanups

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 20 quoted lines
> 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 t0000
>>> 	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. t0000 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 t0000 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.
Show 7 quoted lines
>
> 	Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately
> 	for sub-tests.
>
> Thanks for catching it.
>
> Jonathan
Jeff King· Jan 3, 2014, 01:04 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/3] t0000 cleanups

On Thu, Jan 02, 2014 at 02:28:33PM -0800, Jonathan Nieder wrote:
Show 7 quoted lines
> > This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not
> > leak. t0000 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:
> [...]
Yes, looks good to me.
-Peff

← back to recent threads