Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Derrick Stolee <derrickstolee@github.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD
Date: Wed, 01 Dec 2021 10:38:56 -0800	[thread overview]
Message-ID: <xmqqpmqg1873.fsf@gitster.g> (raw)
In-Reply-To: <patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com> (=?utf-8?B?IsOGdmFyCUFybmZqw7Zyw7A=?= Bjarmason"'s message of "Mon, 29 Nov 2021 21:13:23 +0100")

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the "t1510-repo-setup.sh" test to use a new
> "test_must_be_empty_trace" wrapper, instead of turning on
> "test_untraceable=UnfortunatelyYes".
>
> The only reason the test was incompatible with "-x" was because of
> these "test_must_be_empty" checks, which we can instead instead skip
> if we're running under "set -x".
>
> Skipping the tests is preferable to not having the "-x" output at all,

Even more preferrable is not to skip the tests, no?  Is this because
we capture the standard error stream and do not care where, between
the command being tested and the shell running the command, the
output came from?

> Once that's been removed we can dig deeper and see that we only have
> "BASH_XTRACEFD" due to an earlier attempt to work around the same
> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
> bash, 2017-12-08) follow-up.

If the objective is to work around the "shell trace and program
output are mixed together" issue, BASH_XTRACEFD is doing much more
superb job to "solve" it than "some tests depend on running without
'-x', so punt and skip them" to "work around" it, I would have to
say.

> It's better if our test suite works the same way on all shells, rather
> than getting a passing run under "bash", only to have it fail with
> "-x" under say "dash". As the deleted code shows this is much simpler
> to implement across our supported POSIX shells.

It is much simpler to punt and skip all tests.  You can even have
"if run with -x stop and skip all" in early part of test-lib.sh and
that would "work around" without developers constantly having to
worry about the distinction between test_must_be_empty and the trace
variant.

It is a sad that you have to trade one good thing for another.
Somehow, this change feels like a regression, not an improvement.