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

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