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

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.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: Sat, 12 Jun 2021 23:10:19 -0400	[thread overview]
Message-ID: <CAPig+cR9OnRHYxzVsd6aX=Q_5Fkm4dMxPR2n6kXE+r+cTdw5ug@mail.gmail.com> (raw)
In-Reply-To: <20210612042755.28342-2-congdanhqx@gmail.com>

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.)

> 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).

> +       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.)

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.

> +       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.)

> +       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
> +}