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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd
Date: Sun, 13 Jun 2021 15:36:11 +0200	[thread overview]
Message-ID: <878s3d286c.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20210612042755.28342-2-congdanhqx@gmail.com>

On Sat, Jun 12 2021, Đoàn Trần Công Danh 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.

Thanks for following up on this.

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

I think it's fine to just blindly stick the name into a file in the CWD
as long as it's not "actual" or some other such obviously likely to
conflict name.

The convention you've picked here (I'm not sure if it's existing
already) of naming the temp files after the test lib function name is a
good one.

More generally speaking we have a bunch of helpers that have this
potential issue/bug, in practice it's not a big deal.  A test that's
being overly specific and doing a test_cmp on unbounded "find" output or
whatever is likely buggy anyway.

If it ever becomes a bigger issue we can easily set up two scratch
directories during the test, one for the use of the test itself, and one
for the internals of the test run itself.

> +# 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 () {
> +	local outop outval
> +	local errop errval
> +
> +	while test $# -ge 3
> +	do
> +		case "$1" in
> +		--out)
> +			outop="$2"
> +			outval="$3"
> +			;;
> +		--err)
> +			errop="$2"
> +			errval="$3"
> +			;;

It looks like the end-state of the series leaves us with no user of the
--err option; Maybe it's good to have it anyway for completeness, or
just skip it? ...

> +		*)
> +			break
> +			;;
> +		esac
> +		shift 3
> +	done &&
> +	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 &&

...I think it's better to not pipe to *.err if we haven't requested it,
so under "-v" etc. we can get the stderr.

If we're unifying them I think a better pattern is to only run that "$@"
once, get $?, and then act differently on that in the "!" and ""
cases. It requires less careful reading of the critical function path,
especially with the differing indentation.

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

Let's do that "rm -f" as a "test_when_finished" before we first pipe to
them. It's fine to do that in a test lib function, see e.g. test_config.

We'll get the benefit of preseving these files under -di etc.

> +}
> +
>  test_file_size () {
>  	test "$#" -ne 1 && BUG "1 param"
>  	test-tool path-utils file-size "$1"