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@xxxxxx> 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@xxxxxx>
> > ---
> > 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
