Re: [PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Only total test results are printed out in mptcp_sockopt.sh:
> 
>  PASS: all packets had packet mark set
>  PASS: SOL_MPTCP getsockopt has expected information
>  PASS: TCP_INQ cmsg/ioctl -t tcp
>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>  PASS: TCP_INQ cmsg/ioctl -r tcp
>  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> 
> This patch prints more info for every test result in each test
> group:
> 
>  transfer ipv4                                                [ OK ]
>  mark ipv4                                                    [ OK ]
>  transfer ipv6                                                [ OK ]
>  mark ipv6                                                    [ OK ]
>  PASS: all packets had packet mark set
>  sockopt v4                                                   [ OK ]
>  sockopt v6                                                   [ OK ]
>  PASS: SOL_MPTCP getsockopt has expected information
>  TCP_INQ: -t tcp                                              [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -t tcp
>  TCP_INQ: -6 -t tcp                                           [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>  TCP_INQ: -r tcp                                              [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -r tcp
>  TCP_INQ: -6 -r tcp                                           [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
>  TCP_INQ: -r tcp -t tcp                                       [ OK ]

Please clearly explain why this is interesting, even if it looks obvious.

Here, I don't know if this patch makes sense or not: to me, the output
is now confusing because there is a mix of '[ OK ]' and 'PASS'. Maybe:

- remove all the "PASS: xxx"? → I don't see what it brings more
- (or convert existing "PASS: xxx" and "FAIL: xxx" to use "[ OK ]"? But
there are fewer details)

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 6ed4aa32222f..f84185b5dc9f 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -161,6 +161,7 @@ do_transfer()
>  	wait $spid
>  	local rets=$?
>  
> +	printf "%-50s" "transfer ${ip}"

Best to use a helper to avoid repeating '"%-50s"', and to be able to
change how the titles are displayed from a single place. (same below of
course)

Also, probably looking better if you use 'Transfer' instead of 'transfer'.

>  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
>  		echo " client exit code $retc, server $rets" 1>&2
>  		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
> @@ -169,12 +170,15 @@ do_transfer()
>  		echo -e "\nnetns ${connector_ns} socket stat for ${port}:" 1>&2
>  		ip netns exec ${connector_ns} ss -Menita 1>&2 -o "dport = :$port"
>  
> +		mptcp_lib_print_err "[FAIL]"
>  		mptcp_lib_result_fail "transfer ${ip}"
>  
>  		ret=1
>  		return 1
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"

(here as well, we could use 'mptcp_lib_print_success/fail/skip'
suggested in patch 1/12)

>  
> +	printf "%-50s" "mark ${ip}"

Same here: 'Mark' vs 'mark'.

>  	if [ $local_addr = "::" ];then
>  		check_mark $listener_ns 6 || retc=1
>  		check_mark $connector_ns 6 || retc=1
> @@ -190,8 +194,10 @@ do_transfer()
>  	mptcp_lib_result_code "${rets}" "transfer ${ip}"
>  
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
> +		mptcp_lib_print_ok "[ OK ]"
>  		return 0
>  	fi
> +	mptcp_lib_print_err "[FAIL]"
>  
>  	return 1
>  }
> @@ -220,23 +226,27 @@ do_mptcp_sockopt_tests()
>  	ip netns exec "$ns_sbox" ./mptcp_sockopt
>  	lret=$?
>  
> +	printf "%-50s" "sockopt v4"

Maybe 'SOL_MPTCP getsockopt v4'?

>  	if [ $lret -ne 0 ]; then
>  		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
>  		mptcp_lib_result_fail "sockopt v4"
>  		ret=$lret
>  		return
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"
>  	mptcp_lib_result_pass "sockopt v4"
>  
>  	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
>  	lret=$?
>  
> +	printf "%-50s" "sockopt v6"
>  	if [ $lret -ne 0 ]; then
>  		echo "FAIL: SOL_MPTCP getsockopt (ipv6)" 1>&2
>  		mptcp_lib_result_fail "sockopt v6"
>  		ret=$lret
>  		return
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"
>  	mptcp_lib_result_pass "sockopt v6"
>  }
>  
> @@ -259,6 +269,7 @@ run_tests()
>  
>  do_tcpinq_test()
>  {
> +	printf "%-50s" "TCP_INQ: $*"
>  	ip netns exec "$ns_sbox" ./mptcp_inq "$@"
>  	local lret=$?
>  	if [ $lret -ne 0 ];then
> @@ -267,6 +278,7 @@ do_tcpinq_test()
>  		mptcp_lib_result_fail "TCP_INQ: $*"
>  		return $lret
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"
>  
>  	echo "PASS: TCP_INQ cmsg/ioctl $*"
>  	mptcp_lib_result_pass "TCP_INQ: $*"

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