git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Feb 21, 2022, 21:03 UTC
Message-ID
<20220221210319.GA1658@szeder.dev>
In-Reply-To
<220221.86v8x89ebo.gmgdl@evledraar.gmail.com>
On Mon, Feb 21, 2022 at 08:52:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 20 quoted lines
> 
> [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.

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

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
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 49 of 50 in “Set GIT_TRACE2_EVENT_NESTING in test-lib.sh”
  1. 0/2 Set GIT_TRACE2_EVENT_NESTING in test-lib.shDerrick Stolee via GitGitGadget, Nov 29, 2021
  2. 1/2 test-lib.sh: set GIT_TRACE2_EVENT_NESTINGDerrick Stolee via GitGitGadget, Nov 29, 2021
  3. Ævar Arnfjörð BjarmasonNov 29, 2021
  4. Junio C HamanoNov 29, 2021
  5. Junio C HamanoNov 29, 2021
  6. 2/2 t/t*: remove custom GIT_TRACE2_EVENT_NESTINGDerrick Stolee via GitGitGadget, Nov 29, 2021
  7. Jeff KingNov 29, 2021
  8. Jeff KingNov 29, 2021
  9. Taylor BlauNov 30, 2021
  10. Derrick StoleeNov 30, 2021
  11. Jeff HostetlerNov 30, 2021
  12. Jeff KingDec 1, 2021
  13. test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFDÆvar Arnfjörð Bjarmason, Nov 29, 2021
  14. Eric SunshineNov 29, 2021
  15. Jeff KingNov 30, 2021
  16. Ævar Arnfjörð BjarmasonNov 30, 2021
  17. SZEDER GáborNov 30, 2021
  18. Ævar Arnfjörð BjarmasonDec 1, 2021
  19. Jeff KingDec 1, 2021
  20. Junio C HamanoDec 1, 2021
  21. 0/2 test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFDÆvar Arnfjörð Bjarmason, Dec 1, 2021
  22. 1/2 t1510: remove need for "test_untraceable", retain coverageÆvar Arnfjörð Bjarmason, Dec 1, 2021
  23. SZEDER GáborDec 2, 2021
  24. SZEDER GáborDec 2, 2021
  25. Jeff KingDec 10, 2021
  26. Ævar Arnfjörð BjarmasonDec 10, 2021
  27. SZEDER GáborFeb 6, 2022
  28. 2/2 test-lib.sh: remove the now-unused "test_untraceable" facilityÆvar Arnfjörð Bjarmason, Dec 1, 2021
  29. 0/2 test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFDÆvar Arnfjörð Bjarmason, Dec 10, 2021
  30. 1/2 t1510: remove need for "test_untraceable", retain coverageÆvar Arnfjörð Bjarmason, Dec 10, 2021
  31. 2/2 test-lib.sh: remove the now-unused "test_untraceable" facilityÆvar Arnfjörð Bjarmason, Dec 10, 2021
  32. SZEDER GáborDec 12, 2021
  33. Ævar Arnfjörð BjarmasonDec 12, 2021
  34. SZEDER GáborDec 12, 2021
  35. Junio C HamanoDec 13, 2021
  36. Jeff KingDec 14, 2021
  37. Junio C HamanoDec 15, 2021
  38. Jeff KingDec 15, 2021
  39. Junio C HamanoDec 15, 2021
  40. Ævar Arnfjörð BjarmasonDec 16, 2021
  41. 0/3 test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFDÆvar Arnfjörð Bjarmason, Dec 13, 2021
  42. 1/3 t1510: remove need for "test_untraceable", retain coverageÆvar Arnfjörð Bjarmason, Dec 13, 2021
  43. 2/3 test-lib.sh: remove the now-unused "test_untraceable" facilityÆvar Arnfjörð Bjarmason, Dec 13, 2021
  44. 3/3 test-lib.sh: remove "BASH_XTRACEFD"Ævar Arnfjörð Bjarmason, Dec 13, 2021
  45. SZEDER GáborFeb 21, 2022
  46. Ævar Arnfjörð BjarmasonFeb 22, 2022
  47. SZEDER GáborDec 13, 2021
  48. Ævar Arnfjörð BjarmasonFeb 21, 2022
  49. SZEDER GáborFeb 21, 2022
  50. Ævar Arnfjörð BjarmasonFeb 21, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.