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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 13, 2024, 12:22 UTC
Message-ID: <f5d0510c-d455-5e80-08b7-e08c81df4adb@gmx.de>
In-Reply-To: <xmqq4j7uhfvm.fsf@gitster.g>

Hi Junio,

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

Show 10 quoted lines

> 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