From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Derrick Stolee" <derrickstolee@github.com>,
"Taylor Blau" <me@ttaylorr.com>,
"SZEDER Gábor" <szeder.dev@gmail.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD
Date: Mon, 13 Dec 2021 02:38:33 +0100 [thread overview]
Message-ID: <cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com>
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.
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
next prev parent reply other threads:[~2021-12-13 1:38 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-29 13:47 [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh Derrick Stolee via GitGitGadget
2021-11-29 13:47 ` [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING Derrick Stolee via GitGitGadget
2021-11-29 14:12 ` Ævar Arnfjörð Bjarmason
2021-11-29 18:28 ` Junio C Hamano
2021-11-29 18:32 ` Junio C Hamano
2021-11-29 13:47 ` [PATCH 2/2] t/t*: remove custom GIT_TRACE2_EVENT_NESTING Derrick Stolee via GitGitGadget
2021-11-29 18:40 ` [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh Jeff King
2021-11-29 18:44 ` Jeff King
2021-11-30 0:04 ` Taylor Blau
2021-11-30 15:34 ` Derrick Stolee
2021-11-30 22:43 ` Jeff Hostetler
2021-12-01 19:46 ` Jeff King
2021-11-29 20:13 ` [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-11-29 23:23 ` Eric Sunshine
2021-11-30 21:08 ` Jeff King
2021-11-30 21:50 ` Ævar Arnfjörð Bjarmason
2021-11-30 22:44 ` SZEDER Gábor
2021-12-01 14:06 ` Ævar Arnfjörð Bjarmason
2021-12-01 19:38 ` Jeff King
2021-12-01 18:38 ` Junio C Hamano
2021-12-01 20:11 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason
2021-12-01 20:11 ` [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-02 19:16 ` SZEDER Gábor
2021-12-02 19:28 ` SZEDER Gábor
2021-12-10 9:47 ` Jeff King
2021-12-10 10:08 ` Ævar Arnfjörð Bjarmason
2022-02-06 21:40 ` SZEDER Gábor
2021-12-01 20:11 ` [PATCH v2 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-10 10:07 ` [PATCH v3 0/2] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-12-10 10:07 ` [PATCH v3 1/2] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-10 10:07 ` [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-12 16:32 ` SZEDER Gábor
2021-12-12 17:06 ` Ævar Arnfjörð Bjarmason
2021-12-12 20:14 ` SZEDER Gábor
2021-12-13 18:51 ` Junio C Hamano
2021-12-14 16:43 ` Jeff King
2021-12-15 17:05 ` Junio C Hamano
2021-12-15 17:17 ` Jeff King
2021-12-15 17:32 ` Junio C Hamano
2021-12-16 13:04 ` Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` Ævar Arnfjörð Bjarmason [this message]
2021-12-13 1:38 ` [PATCH v4 1/3] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` [PATCH v4 2/3] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` [PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD" Ævar Arnfjörð Bjarmason
2022-02-21 23:11 ` SZEDER Gábor
2022-02-22 15:14 ` Ævar Arnfjörð Bjarmason
2021-12-13 5:43 ` [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD SZEDER Gábor
2022-02-21 19:52 ` Ævar Arnfjörð Bjarmason
2022-02-21 21:03 ` SZEDER Gábor
2022-02-21 22:41 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).