On 2021-06-12 23:10:19-0400, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
> <congdanhqx@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
