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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 13, 2021, 13:36 UTC
Message-ID: <878s3d286c.fsf@evledraar.gmail.com>
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.

Show 9 quoted lines

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

Show 24 quoted lines

> +# 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? ...

Show 29 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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