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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 13, 2021, 03:10 UTC
Message-ID: <CAPig+cR9OnRHYxzVsd6aX=Q_5Fkm4dMxPR2n6kXE+r+cTdw5ug@mail.gmail.com>
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:

Show 13 quoted lines

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

Show 14 quoted lines

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

Show 20 quoted lines

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

Show 23 quoted lines

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

Show 10 quoted lines

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