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

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
> 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)

Dropped.

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

Updated.

> 
> (...)
> 
> > 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'.

Add a helper print_title.

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

"FAIL:" is removed, keep '1>&2' unchanged.

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

Yes. Replaced by mptcp_lib_pr_fail.

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

"[FAIL]" is dropped here in v6.

> 
> >  	ret=1
> >  
> >  	if [ -n "${1}" ]; then
> 
> ... maybe you can remove this block?
> 
> Cheers,
> Matt