[PATCH] tests: drop use of 'tee' that hides exit status

[PATCH] tests: drop use of 'tee' that hides exit status

[Junio C Hamano]

A few tests have "| tee output" downstream of a git command, and
then inspect the contents of the file.  The net effect is that we
use an extra process, and hide the exit status from the upstream git
command.

In none of these tests, I do not see a reason why we want to hide a
possible failure from these git commands.  Replace the use of tee
with a plain simple redirection.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1001-read-tree-m-2way.sh | 2 +-
 t/t5523-push-upstream.sh    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh
index 88c524f655..48a1550371 100755
--- c/t/t1001-read-tree-m-2way.sh
+++ w/t/t1001-read-tree-m-2way.sh
@@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '
 
 test_expect_success 'a/b vs a, plus c/d case test.' '
 	read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
-	git ls-files --stage | tee >treeMcheck.out &&
+	git ls-files --stage >treeMcheck.out &&
 	test_cmp treeM.out treeMcheck.out
 '
 
diff --git c/t/t5523-push-upstream.sh w/t/t5523-push-upstream.sh
index 1f859ade16..4ad36a31e1 100755
--- c/t/t5523-push-upstream.sh
+++ w/t/t5523-push-upstream.sh
@@ -124,14 +124,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' '
 test_expect_success TTY 'quiet push' '
 	ensure_fresh_upstream &&
 
-	test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output &&
+	test_terminal git push --quiet --no-progress upstream main >output 2>&1 &&
 	test_must_be_empty output
 '
 
 test_expect_success TTY 'quiet push -u' '
 	ensure_fresh_upstream &&
 
-	test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output &&
+	test_terminal git push --quiet -u --no-progress upstream main >output 2>&1 &&
 	test_must_be_empty output
 '
 

Re: [PATCH] tests: drop use of 'tee' that hides exit status

[Rubén Justo]

On Thu, Aug 08, 2024 at 02:19:25PM -0700, Junio C Hamano wrote:
> A few tests have "| tee output" downstream of a git command, and
> then inspect the contents of the file.  The net effect is that we
> use an extra process, and hide the exit status from the upstream git
> command.
> 
> In none of these tests, I do not see a reason why we want to hide a

This double negative caught my attention.  The message is
understandable; perhaps "In none of these tests I see ..." would have
been clearer.

> possible failure from these git commands.  Replace the use of tee
> with a plain simple redirection.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t1001-read-tree-m-2way.sh | 2 +-
>  t/t5523-push-upstream.sh    | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

A simple search only points to these two files:

   $ git grep '\s*git.*|\s*tee' "t/t[0-9]*.sh"
   t/t1001-read-tree-m-2way.sh:400:        git ls-files --stage | tee >treeMcheck.out &&
   t/t5523-push-upstream.sh:127:   test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output &&
   t/t5523-push-upstream.sh:134:   test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output &&

And the following three changes are in line with the result:

> 
> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh
> index 88c524f655..48a1550371 100755
> --- c/t/t1001-read-tree-m-2way.sh
> +++ w/t/t1001-read-tree-m-2way.sh
> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '
>  
>  test_expect_success 'a/b vs a, plus c/d case test.' '
>  	read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
> -	git ls-files --stage | tee >treeMcheck.out &&
> +	git ls-files --stage >treeMcheck.out &&
>  	test_cmp treeM.out treeMcheck.out
>  '
>  
> diff --git c/t/t5523-push-upstream.sh w/t/t5523-push-upstream.sh
> index 1f859ade16..4ad36a31e1 100755
> --- c/t/t5523-push-upstream.sh
> +++ w/t/t5523-push-upstream.sh
> @@ -124,14 +124,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' '
>  test_expect_success TTY 'quiet push' '
>  	ensure_fresh_upstream &&
>  
> -	test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output &&
> +	test_terminal git push --quiet --no-progress upstream main >output 2>&1 &&
>  	test_must_be_empty output
>  '
>  
>  test_expect_success TTY 'quiet push -u' '
>  	ensure_fresh_upstream &&
>  
> -	test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output &&
> +	test_terminal git push --quiet -u --no-progress upstream main >output 2>&1 &&
>  	test_must_be_empty output
>  '
>  

Looks good.

Thanks.

Re: [PATCH] tests: drop use of 'tee' that hides exit status

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> In none of these tests, I do not see a reason why we want to hide a
>
> This double negative caught my attention.  The message is
> understandable; perhaps "In none of these tests I see ..." would have
> been clearer.

You're absolutely right, and it is not just "would have been
clearer"---your fix makes a nonsense double-negation into a correct
sentence.  I'd go with "In any of ... I do not see" while queuing.

Thanks.

Re: [PATCH] tests: drop use of 'tee' that hides exit status

[Johannes Schindelin]

Hi Junio,

On Thu, 8 Aug 2024, Junio C Hamano wrote:

> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh
> index 88c524f655..48a1550371 100755
> --- c/t/t1001-read-tree-m-2way.sh
> +++ w/t/t1001-read-tree-m-2way.sh
> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '
>
>  test_expect_success 'a/b vs a, plus c/d case test.' '
>  	read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
> -	git ls-files --stage | tee >treeMcheck.out &&
> +	git ls-files --stage >treeMcheck.out &&

While this obviously fixes the bug where the test case was incorrectly
allowed to continue after a failing `git ls-files --stage` call, I will
note that I interpret the intention of the `| tee` as showing the output
in the logs in addition to redirecting it to a file for the benefit of
additional checks in the same test case.

I know that I investigate CI failures a lot more than most, therefore many
might disagree that removing such output makes future debugging
potentially a lot harder.

So, what to do here? I don't really know. The easiest option that most
other people would likely be happy with would be to go with the `| tee`
dropping.

A more complex solution (and hence inherently more fragile, which I would
love to avoid) would be to introduce a helper function that redirects the
output but makes sure that it is shown even in case of a failure.
Something like this:

	redirect_and_show () {
		local file="$1"
		shift

		"$@" >"$file"
		local ret=$?

		cat "$file"
		return $ret
	}

As I said, I loathe the complexity of this construct. Hopefully the switch
to the more powerful unit testing framework will make these sort of
considerations moot at some point.

Ciao,
Johannes

Re: [PATCH] tests: drop use of 'tee' that hides exit status

[Jeff King]

On Tue, Aug 13, 2024 at 02:22:11PM +0200, Johannes Schindelin wrote:

> >  test_expect_success 'a/b vs a, plus c/d case test.' '
> >  	read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
> > -	git ls-files --stage | tee >treeMcheck.out &&
> > +	git ls-files --stage >treeMcheck.out &&
> 
> While this obviously fixes the bug where the test case was incorrectly
> allowed to continue after a failing `git ls-files --stage` call, I will
> note that I interpret the intention of the `| tee` as showing the output
> in the logs in addition to redirecting it to a file for the benefit of
> additional checks in the same test case.
> 
> I know that I investigate CI failures a lot more than most, therefore many
> might disagree that removing such output makes future debugging
> potentially a lot harder.

I also look at a lot of CI failures. IMHO this isn't that big an issue.

If "ls-files" succeeds, then the file is fed to test_cmp(), where we
report any differences from the expected output. I find this way more
useful than a dump of the file.

If "ls-files" fails, we won't see the output. But we will see its
stderr, which is probably the more useful stream. Though for cases where
we redirect stderr, I do think it can sometimes be confusing (this is
just not one of those).

But even in that case, we tar up the trash directory of the failing
scripts and save them separately. So the output is still recorded, but
in a less expensive way than dumping it into the log (i.e., it is only
when we see a failure).

Sometimes that isn't sufficient, if later tests overwrite the logs. I
usually run with "--immediate" when locally debugging tests. And I've
very occasionally done the same with CI, pushing up a commit on top to
customize the test environment.

I do wonder if "--immediate" mode would be a better default for CI,
since we expect things to pass. But it it comes at the cost of giving no
information about subsequent tests. So I think it really depends on
whether you are debugging tests you just wrote, or chasing down a flake
or other issue introduced in a script that was not recently touched.

I guess we could record the trash directory state for every failing test
within a script. That would need some cooperation from test-lib.sh, but
probably wouldn't be too hard.

-Peff

Re: [PATCH] tests: drop use of 'tee' that hides exit status

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Thu, 8 Aug 2024, Junio C Hamano wrote:
>
>> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh
>> index 88c524f655..48a1550371 100755
>> --- c/t/t1001-read-tree-m-2way.sh
>> +++ w/t/t1001-read-tree-m-2way.sh
>> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '
>>
>>  test_expect_success 'a/b vs a, plus c/d case test.' '
>>  	read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
>> -	git ls-files --stage | tee >treeMcheck.out &&
>> +	git ls-files --stage >treeMcheck.out &&
>
> While this obviously fixes the bug where the test case was incorrectly
> allowed to continue after a failing `git ls-files --stage` call, I will
> note that I interpret the intention of the `| tee` as showing the output
> in the logs in addition to redirecting it to a file for the benefit of
> additional checks in the same test case.

If we really want to do that, we'd do

    git ls-files --stage >treeMcheck.out &&
    cat treeMcheck.out &&

instead.  You wouldn't lose the exit status, and the output would
show the contents just like "tee" would.

We however tend to also remove a leftover debugging "cat", so the
"cat" is likely to be removed during such a clean-up.

> So, what to do here? I don't really know. The easiest option that most
> other people would likely be happy with would be to go with the `| tee`
> dropping.

As long as we treat the CI as a "batch" environment, the postmortem
tarball of the trash directory is probably the best we can do I can
think of.