Re: [PATCH mptcp-next v5 04/12] selftests: mptcp: print test results with colors

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper mptcp_lib_verify_listener_events() will be added latter in
> mptcp_lib.sh, and be used by mptcp_join.sh and userspace_pm.sh. The
> former prints colored output while the latter is not. It makes sense
> to unify them.

(not sure if we need this explanation about
mptcp_lib_verify_listener_events)
> 
> This patch uses mptcp_lib_print_ok(), _warn(), _err() and _info() helpers
> in scripts diag.sh, mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh,
> simult_flows.sh and userspace_pm.sh to print test results with colors.
> 
> Having colors helps to quickly identify issues when looking at a long
> list of output logs and results.

I think this patch will look better if mptcp_lib_print_success/fail/skip
helpers are used.

In this case, you should not need to use mptcp_lib_print_ok/err/warn,
only _info, right?

If you still need to use '_ok/err/warn' in the new version, please
justify it. But I think these should be "internal" functions, or for
specific need.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 72be905bab1f..c7483902026b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -254,7 +254,8 @@ check_mptcp_disabled()
>  
>  	# net.mptcp.enabled should be enabled by default
>  	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> -		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t\t   [FAIL]"
> +		echo -n -e "net.mptcp.enabled sysctl is not 1 by default"

See the comments on previous patches, would be better with a helper to
print ↑ without 'echo -n -e', and ↓, we would just have
'mptcp_lib_print_fail'.

Same below

> +		mptcp_lib_print_err "\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
>  		ret=1
>  		return 1
> @@ -267,13 +268,15 @@ check_mptcp_disabled()
>  	mptcp_lib_ns_exit "${disabled_ns}"
>  
>  	if [ ${err} -eq 0 ]; then
> -		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t\t   [FAIL]"
> +		echo -n -e "New MPTCP socket cannot be blocked via sysctl"
> +		mptcp_lib_print_err "\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
>  		ret=1
>  		return 1
>  	fi
>  
> -	echo -e "New MPTCP socket can be blocked via sysctl\t\t\t   [ OK ]"
> +	echo -n -e "New MPTCP socket can be blocked via sysctl"
> +	mptcp_lib_print_ok "\t\t\t   [ OK ]"
>  	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
>  	return 0
>  }
> @@ -503,7 +506,7 @@ do_transfer()
>  	fi
>  
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
> -		printf "[ OK ]"
> +		mptcp_lib_print_ok "[ OK ]"
>  		mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
>  	else

Maybe good to add a comment here, e.g.

  # "FAIL" message has been printed before.

>  		mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
> @@ -534,7 +537,6 @@ do_transfer()
>  			"${expect_ackrx}" "${stat_ackrx_now_l}"
>  	fi
>  
> -	echo
>  	cat "$capout"
>  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
>  }
> @@ -708,7 +710,7 @@ EOF
>  		return
>  	fi
>  
> -	echo "INFO: test $msg"
> +	mptcp_lib_print_info "INFO: test $msg"
>  
>  	TEST_COUNT=10000
>  	local extra_args="-o TRANSPARENT"
> @@ -721,12 +723,12 @@ EOF
>  	ip -net "$listener_ns" route del local $local_addr/0 dev lo table 100
>  
>  	if [ $lret -ne 0 ]; then
> -		echo "FAIL: $msg, mptcp connection error" 1>&2
> +		mptcp_lib_print_err "FAIL: $msg, mptcp connection error" 1>&2

(I guess it is fine here to print this to stdout, like all the others,
that's part of the unification I think -- but as mentioned in patch
2/12, we can probably remove this FAIL message, and the PASS one here as
well)

>  		ret=$lret
>  		return 1
>  	fi
>  
> -	echo "PASS: $msg"
> +	mptcp_lib_print_ok "PASS: $msg"
>  	return 0
>  }
>  

(...)

> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index 58cfdaf1db25..79cb377ee0bd 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -187,12 +187,12 @@ do_transfer()
>  	printf "%-16s" " max $max_time "
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
>  	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
> -		echo "[ OK ]"
> +		mptcp_lib_print_ok "[ OK ]"
>  		cat "$capout"
>  		return 0
>  	fi
>  
> -	echo " [FAIL]"
> +	mptcp_lib_print_err " [FAIL]"

I guess we don't need the extra space before '[FAIL]', right?

>  	echo "client exit code $retc, server $rets" 1>&2
>  	echo -e "\nnetns ${ns3} socket stat for $port:" 1>&2
>  	ip netns exec ${ns3} ss -nita 1>&2 -o "sport = :$port"
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index b0cce8f065d8..33bbb0d5807f 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -61,7 +61,7 @@ _printf() {
>  
>  print_title()
>  {
> -	_printf "INFO: %s\n" "${1}"
> +	mptcp_lib_print_info "INFO: ${1}"
>  }
>  
>  # $1: test name
> @@ -72,27 +72,22 @@ print_test()
>  	_printf "%-68s" "${test_name}"
>  }
>  
> -print_results()
> -{
> -	_printf "[%s]\n" "${1}"
> -}
> -
>  test_pass()
>  {
> -	print_results " OK "
> +	mptcp_lib_print_ok "[ OK ]"
>  	mptcp_lib_result_pass "${test_name}"
>  }
>  
>  test_skip()
>  {
> -	print_results "SKIP"
> +	mptcp_lib_print_warn "[SKIP]"
>  	mptcp_lib_result_skip "${test_name}"
>  }
>  
>  # $1: msg
>  test_fail()
>  {
> -	print_results "FAIL"
> +	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"

The error will be repeated twice then, no? ...

>  	ret=1
>  
>  	if [ -n "${1}" ]; then

... maybe you can remove this block?

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