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

On 2021-06-12 23:10:19-0400, Eric Sunshine [off-list ref] wrote:

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
[off-list ref] wrote:

quoted

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

quoted

Signed-off-by: Đoàn Trần Công Danh <redacted>
---

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!

quoted

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

quoted

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

quoted

+       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