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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 21, 2022, 22:41 UTC
Message-ID: <220221.86ee3val4u.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220221210319.GA1658@szeder.dev>

On Mon, Feb 21 2022, SZEDER Gábor wrote:

Show 27 quoted lines

> On Mon, Feb 21, 2022 at 08:52:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> [Junio: If you'd like to pick up this series it still cleanly applies,
>> and merges cleanly with "seen"]
>> 
>> On Mon, Dec 13 2021, SZEDER Gábor wrote:
>> 
>> Sorry about the late reply, things getting lost around the holidays
>> etc.
>> 
>> > On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> A re-arrangement-only change to v3[1]. The previous 2/2 is now split
>> >> into two commits, as requested by SZEDER[2] in the removal of
>> >> BASH_XTRACEFD is now its own commit & the rationale for doing so is
>> >> outlined in detail.
>> >
>> > I'm afraid I wasn't clear.  What I meant was that if we were to remove
>> > BASH_XTRACEFD, then it should be done its own commit.
>> 
>> Aside from whether you think removing it is a good idea, isn't that what
>> this v4 does?
>
> Well, yes, but I now realize that I wasn't sufficiently clear the
> second time around, either, and emphasis doesn't travel well over
> email.  What I meant was that: _if_ at all we were to remove it, then
> it should be a separate patch, but since we should most definitely
> keep it, those hunks should be simply dropped.

I understand, thanks.

Show 11 quoted lines

>> In 1/3 I fix -x under non-BASH_XTRACEFD, 2/3 removes test_untraceable,
>> and 3/3 the use of BASH_XTRACEFD.
>> 
>> > But again: BASH_XTRACEFD is the only simple yet reliable and robust
>> > way to get -x trace from our tests, so do not remove it.
>> 
>> Just to tie off this loose end, I re-read the thread ending in [1] (sent
>> after this reply of yours) and I think my [1] addresses this.
>
> It doesn't at all; "if CI passes without it, then we can remove it" is
> not a convincing argument.

Yes I agree that would be a bad argument, but it's not the one I'm making.

The one I am making is in https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com

I.e. I agree that it's a useful feature, and I wish in the abstract that we could make real use of it.

But that would mean a hard dependency on bash, which I don't think would be acceptable, or something anyone's advocating for.

As long as we're not going down that road I don't see the point in using it.

The only practical use of it in the test suite is to support a special-case I'm making un-special in 1/3, because I wanted "-x" output without needing to run bash. As it turned out it wasn't hard to do, and is consistent with how other tests are written).

So what's your argument for it? I.e. how are we practically going to use it in a way that makes a difference?

I only see us not really using it, because our behavior in practice will 1=1 mirror non-bash shells.

By keeping it around we're only exposing ourselves to edge cases and hiding bugs that we'd like to fix sooner than later anyway (and which are fixed as of this 1/3, and before that mostly were in your earlier series).

So would test code that depended on bash and BASH_XTRACEFD be more "reliable and robust"? Absolutely, you wouldn't need to worry about some interpolated $(pwd) or whatever in a string getting into your -x output when you didn't expect it.

But with how we're using it it's doing the opposite of that. It'll only hide those bugs for non-bash users.

I may be entirely wrong, or perhaps I haven't considered some edge case or trade-off you have in mind. But I'm not able to bridge that gap with your rather terse replies :)

Show 153 quoted lines

>> Maybe you still disagree, but I don't see how that squares with
>> "reliable and robust" here.
>> 
>> I.e. unless we're talking about carrying bash-specific code in the test
>> suite we can't make any real use of the feature, as our tests will need
>> to be compatible with other POSIX shells.
>> 
>> I mean, the code changed in 1/3 *was* that bash-specific code, but as
>> that change shows it was rather easily made non-bash-specific. And
>> unless we think we'll add other bash-specific tests (I don't see why,
>> the cross-shell -x support is rather easy to do) ....
>> 
>> 1. https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com/
>> 
>> >> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/
>> >> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/
>> >> 
>> >> Ævar Arnfjörð Bjarmason (3):
>> >>   t1510: remove need for "test_untraceable", retain coverage
>> >>   test-lib.sh: remove the now-unused "test_untraceable" facility
>> >>   test-lib.sh: remove "BASH_XTRACEFD"
>> >> 
>> >>  t/README              |  3 --
>> >>  t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
>> >>  t/test-lib.sh         | 66 ++++-----------------------------
>> >>  3 files changed, 49 insertions(+), 105 deletions(-)
>> >> 
>> >> Range-diff against v3:
>> >> 1:  7876202c5b0 = 1:  9e7b089dc50 t1510: remove need for "test_untraceable", retain coverage
>> >> -:  ----------- > 2:  60883fd95cb test-lib.sh: remove the now-unused "test_untraceable" facility
>> >> 2:  a7fc794e20d ! 3:  8b5ae33376e test-lib.sh: remove the now-unused "test_untraceable" facility
>> >>     @@ Metadata
>> >>      Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >>      
>> >>       ## Commit message ##
>> >>     -    test-lib.sh: remove the now-unused "test_untraceable" facility
>> >>     +    test-lib.sh: remove "BASH_XTRACEFD"
>> >>      
>> >>     -    In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
>> >>     -    was removed from "t1510-repo-setup.sh" in favor of more narrow
>> >>     -    redirections of the output of specific commands (and not entire
>> >>     -    sub-shells or functions).
>> >>     +    Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
>> >>     +    descriptor 4 under bash.
>> >>      
>> >>     -    This is in line with the fixes in the series that introduced the
>> >>     -    "test_untraceable" facility. See 571e472dc43 (Merge branch
>> >>     -    'sg/test-x', 2018-03-14) for the series as a whole, and
>> >>     -    e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
>> >>     -    subshell, 2018-02-24) for a commit that's in line with the changes in
>> >>     -    the preceding commit.
>> >>     +    When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
>> >>     +    automatically, 2016-05-11) it was needed as a workaround for various
>> >>     +    tests that didn't pass cleanly under "-x".
>> >>      
>> >>     -    We've thus solved the TODO item noted when "test_untraceable" was
>> >>     -    added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
>> >>     -    as untraceable with '-x', 2018-02-24).
>> >>     +    Most of those were later fixed in 71e472dc43 (Merge branch
>> >>     +    'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
>> >>     +    final remaining and removed the "test_untraceable" facility.
>> >>      
>> >>     -    So let's remove the feature entirely. Not only is it currently unused,
>> >>     -    but it actively encourages an anti-pattern in our tests. We should be
>> >>     -    testing the output of specific commands, not entire subshells or
>> >>     -    functions.
>> >>     +    The reason we don't need this anymore is becomes clear from reading
>> >>     +    the rationale in d88785e424a and applying those arguments to the
>> >>     +    current state of the codebase. In particular it said (with "this" and
>> >>     +    "it" referring to the problem of tests failing under "-x"):
>> >>      
>> >>     -    That the "-x" output had to be disabled as a result is only one
>> >>     -    symptom, but even under bash those tests will be harder to debug as
>> >>     -    the subsequent check of the redirected file will be far removed from
>> >>     -    the command that emitted the output.
>> >>     +        "there here isn't a portable or scalable solution to this [...] we
>> >>     +        can work around it by pointing the "set -x" output to our
>> >>     +        descriptor 4"
>> >>      
>> >>     -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >>     +    And finally that:
>> >>      
>> >>     - ## t/README ##
>> >>     -@@ t/README: appropriately before running "make". Short options can be bundled, i.e.
>> >>     - -x::
>> >>     - 	Turn on shell tracing (i.e., `set -x`) during the tests
>> >>     - 	themselves. Implies `--verbose`.
>> >>     --	Ignored in test scripts that set the variable 'test_untraceable'
>> >>     --	to a non-empty value, unless it's run with a Bash version
>> >>     --	supporting BASH_XTRACEFD, i.e. v4.1 or later.
>> >>     - 
>> >>     - -d::
>> >>     - --debug::
>> >>     +        "Automatic tests for our "-x" option may be a bit too meta"
>> >>     +
>> >>     +    Those tests are exactly what we've had since aedffe95250 (travis-ci:
>> >>     +    run tests with '-x' tracing, 2018-02-24), so punting on fixing issues
>> >>     +    with "-x" by using "BASH_XTRACEFD=4" isn't needed anymore, we're now
>> >>     +    committing to maintaining the test suite in a way that won't break
>> >>     +    under "-x".
>> >>     +
>> >>     +    We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
>> >>     +
>> >>     +     1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>> >>     +        using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>> >>     +        into eventually. Tests will pass locally with "bash", but fail in
>> >>     +        CI with "dash" (or under other non-"bash" shells).
>> >>     +
>> >>     +     2) As the amended code in "test_eval_" shows (an amended revert to
>> >>     +        the pre-image of d88785e424a) it's simpler to not have to take
>> >>     +        this "bash" special-case into account.
>> >>     +
>> >>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >>      
>> >>       ## t/test-lib.sh ##
>> >>     -@@ t/test-lib.sh: then
>> >>     - 	exit
>> >>     - fi
>> >>     - 
>> >>     --if test -n "$trace" && test -n "$test_untraceable"
>> >>     --then
>> >>     --	# '-x' tracing requested, but this test script can't be reliably
>> >>     --	# traced, unless it is run with a Bash version supporting
>> >>     --	# BASH_XTRACEFD (introduced in Bash v4.1).
>> >>     --	#
>> >>     --	# Perform this version check _after_ the test script was
>> >>     --	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
>> >>     --	# '--verbose-log', so the right shell is checked and the
>> >>     --	# warning is issued only once.
>> >>     --	if test -n "$BASH_VERSION" && eval '
>> >>     --	     test ${BASH_VERSINFO[0]} -gt 4 || {
>> >>     --	       test ${BASH_VERSINFO[0]} -eq 4 &&
>> >>     --	       test ${BASH_VERSINFO[1]} -ge 1
>> >>     --	     }
>> >>     --	   '
>> >>     --	then
>> >>     --		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
>> >>     --	else
>> >>     --		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
>> >>     --		trace=
>> >>     --	fi
>> >>     --fi
>> >>     - if test -n "$trace" && test -z "$verbose_log"
>> >>     - then
>> >>     - 	verbose=t
>> >>      @@ t/test-lib.sh: else
>> >>       	exec 4>/dev/null 3>/dev/null
>> >>       fi
>> >> -- 
>> >> 2.34.1.1024.g573f2f4b767
>> >> 
>> 
>> o