Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

From: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd
Date: Sun, 13 Jun 2021 14:36:48 +0700	[thread overview]
Message-ID: <YMW1kH4PZWx2NWjW@danh.dev> (raw)
In-Reply-To: <CAPig+cR9OnRHYxzVsd6aX=Q_5Fkm4dMxPR2n6kXE+r+cTdw5ug@mail.gmail.com>

On 2021-06-12 23:10:19-0400, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
> <congdanhqx@gmail.com> wrote:
> > In Git project, we have multiple occasions that requires checking number
> > of lines of text in stdout and/or stderr of a command. One of such
> > example is t6400, which checks number of files in various states.
> >
> > Some of those commands are Git command, and we would like to check their
> > exit status.  In some of those checks, we pipe the stdout of those
> > commands to "wc -l" to check for line count, thus loosing the exit status.
> >
> > Introduce a helper function to check for number of lines in stdout and
> > stderr from those commands.
> >
> > This helper will create 2 temporary files in process, thus it may affect
> > output of some checks.
> 
> If the presence of these files is a concern, I wonder if it would make
> sense to turn these into dot-files (leading dot in name) or shove them
> into the .git/ directory? (Not necessarily an actionable comment; just
> tossing around some thoughts.)

The presence of those files is concern to some command likes:

* git ls-files -o; or
* ls -a

Shoving into ".git" would be actionable if we are testing inside a git
repository. Should we testing outside, we don't have that luxury.

I think we can accept that limitation (only allow this helper inside
a Git worktree to avoid surpising behavior).

> 
> > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> > ---
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > @@ -817,6 +817,70 @@ test_line_count () {
> > +# test_line_count_cmd checks the number of lines of captured stdout and/or
> > +# stderr of a command.
> > +#
> > +# NOTE: this helper function will create 2 temporary files named:
> > +# * test_line_count_cmd_.out; and
> > +# * test_line_count_cmd_.err
> > +#
> > +# And this helper function will remove those 2 files if the check is succeed.
> > +# In case of failure, those files will be preserved.
> > +test_line_count_cmd () {
> 
> It would be good to have some usage information in the above comment
> so that people aren't forced to consult the implementation to learn
> what options this function takes. At minimum, it should mention
> `--out`, `--err`, and `!`, and should explain the arguments each
> option takes (even if just through examples).

Make sense!

> 
> > +       local outop outval
> > +       local errop errval
> > +
> > +       while test $# -ge 3
> > +       do
> > +               case "$1" in
> > +               --out)
> > +                       outop="$2"
> > +                       outval="$3"
> > +                       ;;
> > +               --err)
> > +                       errop="$2"
> > +                       errval="$3"
> > +                       ;;
> > +               *)
> > +                       break
> > +                       ;;
> > +               esac
> > +               shift 3
> > +       done &&
> 
> This is really minor, but if test_line_count_cmd() ever learns some
> new option and that option does not consume two arguments, then the
> `shift 3` at the end of the `case/esac` will need to be adjusted in
> some fashion. It might make more sense, therefore, to perform the
> `shift 3` closer to where it is needed (that is, in the `--out` case
> and in the `--err` case) rather than delaying it as is done here. (Not
> necessarily worth a re-roll.)

Make sense, too. I think I'll add another leg here for "-*" to avoid
potential break when adding/removing options. Not that I imagine about
new options, but I can't think of any command starts with "-"

> Another minor comment: Since you're &&-chaining everything else in the
> function, it wouldn't hurt to also do so for the `local` declarations
> and the assignments within each `case` arm, and to chain `esac` with
> `shift 3`. Doing so could help some future programmer who might (for
> some reason) insert code above the `while` loop, thinking that a
> failure in the new code will abort the function, but not realizing
> that the &&-chain is not intact in this area of the code.

Will do, too!

> > +       if test $# = 0 ||
> > +          { test "x$1" = "x!" && test $# = 1 ; }
> > +       then
> > +               BUG "test_line_count_cmd: no command to be run"
> > +       fi &&
> > +       if test -z "$outop$errop"
> > +       then
> > +               BUG "test_line_count_cmd: check which stream?"
> > +       fi &&
> > +
> > +       if test "x$1" = "x!"
> > +       then
> > +               shift &&
> > +               if "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> > +               then
> > +                       echo "error: '$@' succeed!"
> > +                       return 1
> > +               fi
> > +       elif ! "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> > +       then
> > +               echo "error: '$@' run into failure!"
> > +               return 1
> > +       fi &&
> 
> Do we care that the "!" negated case doesn't have the same semantics
> as test_must_fail()? If we do care, then should there be a way to tell
> the function whether we want test_must_fail() semantics or `!`
> semantics (i.e. whether we're running a Git command or a non-Git
> command) or should it infer it on its own? (These are genuine
> questions -- not necessarily requests for changes -- as I'm trying to
> reason through the implications of this implementation choice.)

I think that we shouldn't care, in my mind, I would let users to chain
test_line_count and test_must_fail if they do care about that sematic.
So, we have the freedom to use test_line_count_cmd with both Git and
non-Git commands.  E.g:

---------8<---------------

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ad4746d899..94c8cfc9f2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -288,9 +288,8 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 '
 
 test_expect_success 'OPT_CALLBACK() and callback errors work' '
-	test_must_fail test-tool parse-options --no-length >output 2>output.err &&
-	test_must_be_empty output &&
-	test_must_be_empty output.err
+	test_line_count_cmd --out = 0 --err = 0 \
+		test_must_fail test-tool parse-options --no-length
 '
 
 cat >expect <<\EOF
------------>8--------------
> 
> > +       if test -n "$outop"
> > +       then
> > +               test_line_count "$outop" "$outval" test_line_count_cmd_.out
> > +       fi &&
> > +       if test -n "$errop"
> > +       then
> > +               test_line_count "$errop" "$errval" test_line_count_cmd_.err
> > +       fi &&
> > +       rm -f test_line_count_cmd_.out test_line_count_cmd_.err
> > +}

-- 
Danh
