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

From: Jeff King <peff@peff.net>
Date: Aug 13, 2024, 12:59 UTC
Message-ID: <20240813125900.GD968816@coredump.intra.peff.net>
In-Reply-To: <f5d0510c-d455-5e80-08b7-e08c81df4adb@gmx.de>

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

Show 14 quoted lines

> >  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