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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.

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

> 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