From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Mon, 21 Jun 2021 09:08:07 GMT
Subject: Re: [PATCH v3 1/4] test-lib-functions: introduce test_line_count_cmd
Message-ID: <54bf7756-5578-4fe4-dbe5-f63b368c4d63@gmail.com>
In-Reply-To: <20210619013035.26313-2-congdanhqx@gmail.com>

On 19/06/2021 03:30, Đoàn Trần Công Danh wrote:
> In the Git project, we have multiple instances that requires

s/requires/require/

> checking number of lines of text in the stdout of a command.
> One of such examples is t6400, that 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 count the number lines, thus losing
> the exit status.
> 
> Introduce a helper function to check for the number of lines in stdout
> from those commands.
> 
> This helper will create a temporary file in the process, thus it may
> affect the output of some checks.
> 
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>   t/test-lib-functions.sh | 80 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index f0448daa74..e055a4f808 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -845,6 +845,86 @@ test_line_count () {
>   	fi
>   }
>   
> +# test_line_count_cmd checks the exit status, and the number of lines in
> +# the captured stdout of a command.
> +#
> +# SYNOPSIS:
> +#
> +#	test_line_count_cmd <binop> <value> [!] cmd [args...]
> +#
> +# Expect succeed exit status when running
> +#
> +#	cmd [args...]
> +#
> +# then, run sh's "test <# of lines in stdout> <binop> <value>"
> +#
> +# OPTIONS:
> +# !:
> +#	Instead of expecting "cmd [args...]" succeed, expect its failure.
> +#	Note, if the command under testing is "git",
> +#	test_must_fail should be used instead of "!".
> +#
> +# EXAMPLE:
> +#	test_line_count_cmd -ge 10 git tag --no-contains v1.0.0
> +#	test_line_count_cmd -le 10 ! grep some-text a-file
> +#	test_line_count_cmd = 0 test_must_fail git rev-parse --verify abcd1234
> +#
> +# NOTE:
> +# * a temporary file named test_line_count_cmd_.out will be created under
> +# $TRASH_DIRECTORY/.git/trash iff $TRASH_DIRECTORY/.git/ exists.
> +# Otherwise, created in $TRASH_DIRECTORY. This temporary file will be
> +# cleaned by test_when_finished
> +test_line_count_cmd () {
> +	{
> +		local outop outval outfile
> +		local expect_failure actual_failure
> +		local trashdir="$TRASH_DIRECTORY"
> +
> +		if test -d "$TRASH_DIRECTORY/.git"
> +		then
> +			trashdir="$TRASH_DIRECTORY/.git/trash" &&
> +			mkdir -p "$trashdir"
> +		fi &&
> +		if test $# -lt 3
> +		then
> +			BUG "missing <binary-ops> and <value>"

Nit: s/binary-ops/binop/ for consistency with documentation comment
above.  Also, technically the invocation of test_line_count_cmd could be
missing any of its required three parameters, including "cmd".  How
about something similar to the call to BUG in test_i18ngrep:

	BUG "too few parameters to test_line_count_cmd"

?

> +		fi &&
> +		outop="$1" &&
> +		outval="$2" &&
> +		shift 2 &&
> +		outfile="$trashdir/test_line_count_cmd_.out" &&
> +		if test "x$1" = "x!"
> +		then
> +			shift &&
> +			expect_failure=yes
> +		fi &&
> +		if test $# = 0
> +		then
> +			BUG "test_line_count_cmd: no command to be run"
> +		else
> +			test_when_finished "rm -f '$outfile'" &&
> +			exec 8>"$outfile"
> +			# We need to redirect stderr to &9,
> +			# and redirect this function's 9>&2
> +			# in order to not messed with -x
> +			if ! "$@" >&8 2>&9
> +			then
> +				actual_failure=yes
> +			fi
> +		fi 8>&1 &&
> +		case "$expect_failure,$actual_failure" in
> +		yes,)
> +			echo >&4 "error: '$@' succeed!" &&

It seems that function error() could be used here and below instead of
"echo >&4".

s/succeed/succeeded/ --- it might be worth borrowing wording from
test_must_fail().  Something like:

	error "test_line_count_cmd: command succeeded: '$@'"

> +			return 1
> +			;;
> +		,yes)
> +			echo >&4 "error: '$@' run into failure!" &&
> +			return 1
> +		esac &&

Missing ";;" in the last branch of `case`.

> +		test_line_count "$outop" "$outval" "$outfile" >&4
> +	} 9>&2 2>&4
> +}
> +
>   test_file_size () {
>   	test "$#" -ne 1 && BUG "1 param"
>   	test-tool path-utils file-size "$1"
>