Re: [PATCH mptcp-next v7 7/8] selftests: mptcp: print test results with counters

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper mptcp_lib_print_title(), a wrapper of
> mptcp_lib_inc_test_counter() and mptcp_lib_pr_title_counter(), to
> print out test counter in each test result and increase the counter.
> Use this helper to print out test counters for every tests in diag.sh,
> mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
> and userspace_pm.sh.

(...)

> Having test counters helps to quickly identify issues when looking at a
> long list of output logs and results.

Nice!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 77b0b9d5aca0..096ff8941c5b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -131,7 +131,6 @@ ns2=""
>  ns3=""
>  ns4=""
>  
> -TEST_COUNT=0
>  TEST_GROUP=""
>  
>  # This function is used in the cleanup trap
> @@ -253,6 +252,7 @@ check_mptcp_disabled()
>  	local disabled_ns
>  	mptcp_lib_ns_init disabled_ns
>  
> +	mptcp_lib_print_title "New MPTCP socket can be blocked via sysctl"

Because you have 2 exceptions (larger titles), maybe best to use a helper:

  print_larger_title() {
      # here we don't have the time, a bit longer for the alignment
      MPTCP_LIB_TEST_FORMAT="%02u %-69s" \
          mptcp_lib_print_title "${@}"
  }

>  	# 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[ FAIL ]"

(...)

> @@ -830,6 +828,7 @@ stop_if_error()
>  	fi
>  }
>  
> +MPTCP_LIB_TEST_FORMAT="%02u %-69s"

... so you don't have to set it here ...

>  make_file "$cin" "client"
>  make_file "$sin" "server"
>  
> @@ -837,7 +836,7 @@ check_mptcp_disabled
>  
>  stop_if_error "The kernel configuration is not valid for MPTCP"
>  
> -printf "%-69s" "Validating network environment with pings"
> +mptcp_lib_print_title "Validating network environment with pings"

(print_larger_title → clearer)

>  for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
>  	do_ping "$ns1" $sender 10.0.1.1
>  	do_ping "$ns1" $sender dead:beef:1::1
> @@ -860,6 +859,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
>  
>  stop_if_error "Could not even run ping tests"
>  echo "[ OK ]"
> +MPTCP_LIB_TEST_FORMAT="%02u %-50s"

... and here, just to handle 2 exceptions, and it is not clear which
format will be used.

>  
>  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
>  echo -n "INFO: Using loss of $tc_loss "

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index ae59136b5bb4..a797e13d3fe7 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=$?
>  
> +	mptcp_lib_print_title "Transfer ${ip}"

Please add a note in the commit message explaining that
'mptcp_sockopt.sh' now display more detailed results + why (what you had
in a former patch from v6, merged here).

(Note: to replace patch 5/8, you can also use '${ip:2}' here, maybe
prettier? A detail...)

>  	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
> @@ -174,7 +175,9 @@ do_transfer()
>  		ret=1
>  		return 1
>  	fi
> +	echo "[ OK ]"
>  
> +	mptcp_lib_print_title "Mark ${ip}"

(same here for '${ip:2}' if you want to)

>  	if [ $local_addr = "::" ];then
>  		check_mark $listener_ns 6 || retc=1
>  		check_mark $connector_ns 6 || retc=1
> @@ -190,8 +193,10 @@ do_transfer()
>  	mptcp_lib_result_code "${rets}" "transfer ${ip}"
>  
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
> +		echo "[ OK ]"
>  		return 0
>  	fi
> +	echo "FAIL: Mark ${ip}"

I don't think you need to repeat 'Mark ${ip}', it is already in the
title, no?

>  
>  	return 1
>  }

(...)

> @@ -314,15 +324,7 @@ trap cleanup EXIT
>  run_tests $ns1 $ns2 10.0.1.1
>  run_tests $ns1 $ns2 dead:beef:1::1
>  
> -if [ $ret -eq 0 ];then
> -	echo "PASS: all packets had packet mark set"
> -fi
> -
>  do_mptcp_sockopt_tests
> -if [ $ret -eq 0 ];then
> -	echo "PASS: SOL_MPTCP getsockopt has expected information"
> -fi

Please also mention in the commit message that it no longer displays
'PASS:', because it is duplicated info now that the detailed are displayed.

> -
>  do_tcpinq_tests
>  
>  mptcp_lib_result_print_all_tap

(...)

> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index d5f8521b88d5..212782301c6f 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh

(...)

> @@ -205,6 +203,12 @@ do_transfer()
>  	return 1
>  }
>  
> +print_title()
> +{
> +	MPTCP_LIB_TEST_FORMAT="%02u %-55s"

Same here, can be set once above, at the beginning of the file (with a
small comment explaining why we need a bit more space: because we have
more to display :) )

> +	mptcp_lib_print_title "${*}"
> +}
> +
>  run_test()
>  {
>  	local rate1=$1
> @@ -239,7 +243,7 @@ run_test()
>  	# completion (see mptcp_connect): 200ms on each side, add some slack
>  	time=$((time + 400 + slack))
>  
> -	printf "%-60s" "$msg"
> +	print_title "$msg"
>  	do_transfer $small $large $time
>  	lret=$?
>  	mptcp_lib_result_code "${lret}" "${msg}"
> @@ -249,7 +253,7 @@ run_test()
>  	fi
>  
>  	msg+=" - reverse direction"
> -	printf "%-60s" "$(echo -n ${msg} | cut -c1-54)"
> +	print_title "$(echo -n ${msg} | cut -c1-54)"

As discussed on a previous patch, I think it is best not to cut the
title. (and if we had to, best to do that in the helper, not here)

>  	do_transfer $large $small $time
>  	lret=$?
>  	mptcp_lib_result_code "${lret}" "${msg}"

(...)

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