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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2021, 18:38 UTC
Message-ID: <xmqqpmqg1873.fsf@gitster.g>
In-Reply-To: <patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com>

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

Show 9 quoted lines

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

Show 5 quoted lines

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