From: Patrick Steinhardt <ps@pks•im>
To: Eric Sunshine <sunshine@sunshineco•com>
Cc: git@vger•kernel.org, Phillip Wood <phillip.wood123@gmail•com>,
Junio C Hamano <gitster@pobox•com>,
Karthik Nayak <karthik.188@gmail•com>,
Ramsay Jones <ramsay@ramsayjones•plus.com>,
Eli Schwartz <eschwartz@gentoo•org>,
Todd Zullinger <tmz@pobox•com>
Subject: Re: [PATCH v2 1/6] t: fix cases where output breaks TAP format
Date: Wed, 28 May 2025 17:55:13 +0200 [thread overview]
Message-ID: <aDcx4dXe12tRUyYd@pks.im> (raw)
In-Reply-To: <CAPig+cSYhY+LQ5pD+a1O16Rxwo_js45WqfcW8wtC2daYmNyMCQ@mail.gmail.com>
On Tue, May 27, 2025 at 03:47:16PM -0400, Eric Sunshine wrote:
> On Tue, May 27, 2025 at 10:03 AM Patrick Steinhardt <ps@pks•im> wrote:
> > The TAP format does not allow arbitrary output outside of a specific
> > test case. If a test suite wants to print any such diagnostic output,
> > then this output has to be prefixed with "#" to mark it accordingly.
> > A bunch of our tests generate output outside of `test_expect_*`
> > testcases anyway without such a mark, which breaks strict TAP parsers.
> >
> > Upon further inspection, all of the output generated by such tests is
> > rather uninteresting. Refactor them so that we don't break the TAP
> > format.
>
> Nit: Can we avoid the word "refactor" for changes such as those made
> by this patch which clearly are not refactoring[*].
>
> [*]: From Wikipedia: "... code refactoring is the process of
> restructuring existing source code—changing the factoring—without
> changing its external behavior."
Fair. We can say "adapt" instead.
> > Signed-off-by: Patrick Steinhardt <ps@pks•im>
> > ---
> > diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
> > @@ -30,7 +30,7 @@ setup_repo() {
> > test_repo=test
> > push_repo() {
> > - test_create_repo $test_repo
> > + test_create_repo $test_repo >/dev/null
> > cd $test_repo
> > setup_repo
>
> Yuck, but certainly the simplest "fix" in this particular case
> considering that, ultimately, this entire script ought to be reworked
> since it cd's around outside of tests with abandon. It would be nice
> to see this script get overhauled eventually but such an undertaking
> doesn't need to be part of this patch series.
Yeah, a bunch of test scripts fall into this category indeed.
> > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> > @@ -48,7 +48,7 @@ commit_file () {
> > -test_create_repo sm1 &&
> > +test_create_repo sm1 >/dev/null &&
> > add_file . foo >/dev/null
> >
> > head1=$(add_file sm1 foo1 foo2)
>
> Unlike the case with t1007, in which the entire script needs an
> overhaul, it is much easier to fix the problems in this script without
> papering over them via ">/dev/null". In particular, it would be
> preferable to resolve the issue by wrapping test_expect_success around
> the code which currently resides outside of any test. So, for example,
> the above could become:
>
> test_expect_success 'setup submodule 1' '
> test_create_repo sm1 &&
> add_file . foo &&
> head1=$(add_file sm1 foo1 foo2) &&
> fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> '
>
> Note that I also dropped the ">/dev/null" redirect from the add_file()
> invocation.
>
> The same comment applies to similar changes made by this patch to
> other scripts, such as t4060, t7401.
Yes, it isn't particularly hard. But it does result in a bunch of
shuffling that makes the patch way harder to read.
> > diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
> > @@ -7,12 +7,17 @@ test_description='Clone repositories with non ASCII paths'
> > -ISO8859="$(printf "$ISO8859_ESCAPED")" &&
> > -echo content123 >"$ISO8859" &&
> > -rm "$ISO8859" || {
> > +test_lazy_prereq FS_ACCEPTS_ISO_8859_1 '
> > + ISO8859="$(printf "$ISO8859_ESCAPED")" &&
> > + echo content123 >"$ISO8859" 2>/dev/null &&
> > + rm "$ISO8859"
> > +'
>
> Was the problem here that the `echo content123 > "$..."` was
> potentially spitting out an error message to stderr, thus you had to
> redirect it to /dev/null to silence it?
Ah, this redirect is not required anymore. I had it in a previous
version due to the exact problem that you mentioned, that echo spit out
an error.
> If so, did the file get created in the error case? What I'm wondering
> is whether you also should use `rm -f` when removing the file.
The idea here is that some systems fail to create the file in the first
place, which will cause the echo to fail. In that case, the file has not
been created either, so there is no need to remove it.
Patrick
next prev parent reply other threads:[~2025-05-28 15:55 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-06 10:59 [PATCH 0/4] meson: parse TAP output generated by our tests Patrick Steinhardt
2025-05-06 10:59 ` [PATCH 1/4] t: fix cases where output breaks TAP format Patrick Steinhardt
2025-05-06 13:17 ` Phillip Wood
2025-05-07 6:52 ` Patrick Steinhardt
2025-05-07 10:12 ` Phillip Wood
2025-05-14 18:51 ` Karthik Nayak
2025-05-06 10:59 ` [PATCH 2/4] t/test-lib: don't print shell traces to stdout Patrick Steinhardt
2025-05-06 10:59 ` [PATCH 3/4] meson: introduce kwargs variable for tests Patrick Steinhardt
2025-05-15 7:39 ` Karthik Nayak
2025-05-06 10:59 ` [PATCH 4/4] meson: parse TAP output generated by our tests Patrick Steinhardt
2025-05-15 7:48 ` Karthik Nayak
2025-05-15 8:20 ` Patrick Steinhardt
2025-05-15 11:35 ` Karthik Nayak
2025-05-06 12:29 ` [PATCH 0/4] " Patrick Steinhardt
2025-05-07 7:06 ` Patrick Steinhardt
2025-05-21 10:57 ` Patrick Steinhardt
2025-05-21 11:56 ` Hridoy Ahmed
2025-05-21 16:06 ` Junio C Hamano
2025-05-21 21:26 ` Junio C Hamano
2025-05-23 10:03 ` Patrick Steinhardt
2025-05-23 15:00 ` Patrick Steinhardt
2025-05-23 15:58 ` Junio C Hamano
2025-05-23 16:40 ` Ramsay Jones
2025-05-23 16:53 ` Ramsay Jones
2025-05-23 19:33 ` Junio C Hamano
2025-05-26 12:44 ` Patrick Steinhardt
2025-05-26 13:31 ` Phillip Wood
2025-05-26 14:23 ` Todd Zullinger
2025-05-26 13:54 ` Eli Schwartz
2025-05-26 13:59 ` Patrick Steinhardt
2025-05-27 16:04 ` Junio C Hamano
2025-05-28 12:27 ` Patrick Steinhardt
2025-05-27 13:36 ` Junio C Hamano
2025-05-27 14:02 ` [PATCH v2 0/6] " Patrick Steinhardt
2025-05-27 14:02 ` [PATCH v2 1/6] t: fix cases where output breaks TAP format Patrick Steinhardt
2025-05-27 19:47 ` Eric Sunshine
2025-05-28 15:55 ` Patrick Steinhardt [this message]
2025-05-28 20:14 ` Eric Sunshine
2025-05-30 7:50 ` Patrick Steinhardt
2025-05-27 14:02 ` [PATCH v2 2/6] t/test-lib: don't print shell traces to stdout Patrick Steinhardt
2025-05-27 19:47 ` Junio C Hamano
2025-05-28 15:55 ` Patrick Steinhardt
2025-05-27 14:02 ` [PATCH v2 3/6] t/test-lib: fix TAP format for BASH_XTRACEFD warning Patrick Steinhardt
2025-05-27 14:02 ` [PATCH v2 4/6] t7815: fix unexpectedly passing test on macOS Patrick Steinhardt
2025-05-27 14:02 ` [PATCH v2 5/6] meson: introduce kwargs variable for tests Patrick Steinhardt
2025-05-27 14:02 ` [PATCH v2 6/6] meson: parse TAP output generated by our tests Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 00/10] " Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 01/10] t: stop announcing prereqs Patrick Steinhardt
2025-05-31 21:00 ` Karthik Nayak
2025-05-30 13:31 ` [PATCH v3 02/10] t: silence output from `test_create_repo()` Patrick Steinhardt
2025-05-30 21:16 ` Eric Sunshine
2025-05-30 13:31 ` [PATCH v3 03/10] t9822: use prereq to check for ISO-8859-1 support Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 04/10] t983*: use prereq to check for Python-specific git-b4(1) support Patrick Steinhardt
2025-05-30 14:08 ` Todd Zullinger
2025-05-30 14:21 ` Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 05/10] t/test-lib: don't print shell traces to stdout Patrick Steinhardt
2025-05-31 21:21 ` Karthik Nayak
2025-05-30 13:31 ` [PATCH v3 06/10] t/test-lib: fix TAP format for BASH_XTRACEFD warning Patrick Steinhardt
2025-05-31 21:25 ` Karthik Nayak
2025-05-30 13:31 ` [PATCH v3 07/10] t7815: fix unexpectedly passing test on macOS Patrick Steinhardt
2025-05-31 21:28 ` Karthik Nayak
2025-06-01 9:19 ` Kristoffer Haugsbakk
2025-06-02 6:19 ` Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 08/10] test-lib: fail on unexpectedly passing tests Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 09/10] meson: introduce kwargs variable for tests Patrick Steinhardt
2025-05-30 13:31 ` [PATCH v3 10/10] meson: parse TAP output generated by our tests Patrick Steinhardt
2025-05-31 21:37 ` [PATCH v3 00/10] " Karthik Nayak
2025-06-02 6:44 ` [PATCH v4 " Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 01/10] t: stop announcing prereqs Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 02/10] t: silence output from `test_create_repo()` Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 03/10] t9822: use prereq to check for ISO-8859-1 support Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 04/10] t983*: use prereq to check for Python-specific git-p4(1) support Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 05/10] t/test-lib: don't print shell traces to stdout Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 06/10] t/test-lib: fix TAP format for BASH_XTRACEFD warning Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 07/10] t7815: fix unexpectedly passing test on macOS Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 08/10] test-lib: fail on unexpectedly passing tests Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 09/10] meson: introduce kwargs variable for tests Patrick Steinhardt
2025-06-02 6:44 ` [PATCH v4 10/10] meson: parse TAP output generated by our tests Patrick Steinhardt
2025-06-02 7:40 ` [PATCH v4 00/10] " Karthik Nayak
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aDcx4dXe12tRUyYd@pks.im \
--to=ps@pks$(echo .)im \
--cc=eschwartz@gentoo$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=phillip.wood123@gmail$(echo .)com \
--cc=ramsay@ramsayjones$(echo .)plus.com \
--cc=sunshine@sunshineco$(echo .)com \
--cc=tmz@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox