Re: [PATCH mptcp-next v5 08/12] selftests: mptcp: move test_fail out of check_expected_one

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch moves test_fail out of check_expected_one(), since test_fail
> is a private function in userspace_pm.sh, and check_expected_one will be
> exported in mptcp_lib.sh in the next commit.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 27f308601005..473639a8009f 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -247,7 +247,7 @@ check_expected_one()
>  
>  	if [ "${prev_ret}" = "0" ]
>  	then
> -		test_fail

I think, the goal to do it here was to print "FAIL", then the details.

> +		return 2

Just return 1, no?

>  	fi
>  
>  	_printf "\tExpected value for '%s': '%s', got '%s'.\n" \

What's the output now in case of issue? Does it still make sense to use
the '\t' here at the beginning?

> @@ -263,13 +263,20 @@ check_expected()
>  
>  	for var in "${@}"
>  	do
> -		check_expected_one "${var}" "${rc}" || rc=1
> +		check_expected_one "${var}" "${rc}" || rc="${?}"
> +		if [ "${rc}" -eq 2 ]
> +		then
> +			break

Why breaking? If you break, you only print one error, not all like before.

Please check the behaviour in case of error before and after the
modification.

> +		fi
>  	done
>  
>  	if [ ${rc} -eq 0 ]
>  	then
>  		test_pass
>  		return 0
> +	elif [ "${rc}" -eq 2 ]
> +	then
> +		test_fail

Just 'test_fail' after the 'if'.

>  	fi
>  
>  	return 1

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.