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

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, 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: Mon, 29 Nov 2021 18:23:43 -0500	[thread overview]
Message-ID: <CAPig+cQ-WsuLLfUiseEithhGXWRgS=nB95BNe+k8iSPCCxJ2xw@mail.gmail.com> (raw)
In-Reply-To: <patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com>

On Mon, Nov 29, 2021 at 5:20 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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".

s/instead instead/instead/

> Skipping the tests is preferable to not having the "-x" output at all,
> as it's much easier to debug the test. The result loss of test
> coverage is minimal. If someone is adjusting a "test_must_be_empty"
> test a failure might go away under "-x", but the new "say" we emit
> here should highlight that appropriately.
>
> Since the only user of "test_untraceable" is gone, we can remove that,
> not only isn't it used now, but I think the rationale for using it in
> the future no longer applies.
>
> We'll be better off by using a simple wrapper like the new
> "test_must_be_empty_trace". See 58275069288 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24) and 5fc98e79fc0 (t: add means to
> disable '-x' tracing for individual test scripts, 2018-02-24) for the
> addition of "test_untraceable".
>
> 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.
>
> I.e. we had redirection in "test_eval_" to get more relevant trace
> output under bash, which in turn was only needed because
> BASH_XTRACEFD=1 was set, which in turn was trying to work around test
> failures under "set -x".
>
> 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.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>