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

[Geliang Tang <geliang@kernel.org>]

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

This patch uses to match the output of this script:

INFO: PASS: all packets had packet mark set
INFO: PASS: SOL_MPTCP getsockopt has expected information
INFO: PASS: TCP_INQ cmsg/ioctl -t tcp
INFO: PASS: TCP_INQ cmsg/ioctl -6 -t tcp
INFO: PASS: TCP_INQ cmsg/ioctl -r tcp
INFO: PASS: TCP_INQ cmsg/ioctl -6 -r tcp
INFO: PASS: TCP_INQ cmsg/ioctl -r tcp -t tcp

with the test results of it:

ok 1 - mptcp_sockopt: mark ipv4
ok 2 - mptcp_sockopt: transfer ipv4
ok 3 - mptcp_sockopt: mark ipv6
ok 4 - mptcp_sockopt: transfer ipv6
ok 5 - mptcp_sockopt: sockopt v4
ok 6 - mptcp_sockopt: sockopt v6
ok 7 - mptcp_sockopt: TCP_INQ: -t tcp
ok 8 - mptcp_sockopt: TCP_INQ: -6 -t tcp
ok 9 - mptcp_sockopt: TCP_INQ: -r tcp
ok 10 - mptcp_sockopt: TCP_INQ: -6 -r tcp
ok 11 - mptcp_sockopt: TCP_INQ: -r tcp -t tcp

This patch is prepared for the coming printing counter patch, otherwise
the two output results may have different serial numbers, which can be
confusing.

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

Add a new helper print_title() to do this.

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

Updated in v6.

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


Replaced by mptcp_lib_pr_ok.

> 
> >  
> > +	printf "%-50s" "mark ${ip}"
> 
> Same here: 'Mark' vs 'mark'.

Yes, updated.

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

Updated.

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