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

[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>
> > 
> > This patch adds a new helper mptcp_lib_print_test_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.
> > 
> > Each output looks like:
> > 
> > diag.sh
> >  01 no msk on netns creation                          [  OK  ]
> >  02 listen match for dport 10000                      [  OK  ]
> >  03 listen match for sport 10000                      [  OK  ]
> >  04 listen match for saddr and sport                  [  OK  ]
> >  05 all listen sockets                                [  OK  ]
> > 
> > mptcp_connect.sh
> >  01 New MPTCP socket can be blocked via sysctl                     
> > [ OK ]
> >  INFO: validating network environment with pings
> >  02 ping tests                                                     
> > [ OK ]
> >  INFO: Using loss of 0.16% delay 25 ms reorder .. with delay 6ms on
> > ns3eth4
> >  03 ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP  (duration 116ms)
> > [ OK ]
> >  04 ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP    (duration  33ms)
> > [ OK ]
> >  05 ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP  (duration  25ms)
> > [ OK ]
> >  06 ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP  (duration 128ms)
> > [ OK ]
> >  07 ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP    (duration  31ms)
> > [ OK ]
> > 
> > mptcp_sockopt.sh
> >  01 transfer ipv4                                                [
> > OK ]
> >  02 mark ipv4                                                    [
> > OK ]
> >  03 transfer ipv6                                                [
> > OK ]
> >  04 mark ipv6                                                    [
> > OK ]
> >  PASS: all packets had packet mark set
> >  05 sockopt v4                                                   [
> > OK ]
> >  06 sockopt v6                                                   [
> > OK ]
> >  PASS: SOL_MPTCP getsockopt has expected information
> >  07 TCP_INQ: -t tcp                                              [
> > OK ]
> >  PASS: TCP_INQ cmsg/ioctl -t tcp
> >  08 TCP_INQ: -6 -t tcp                                           [
> > OK ]
> >  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> >  09 TCP_INQ: -r tcp                                              [
> > OK ]
> >  PASS: TCP_INQ cmsg/ioctl -r tcp
> >  10 TCP_INQ: -6 -r tcp                                           [
> > OK ]
> > 
> > pm_netlink.sh
> >  01 defaults addr list                        [ OK ]
> >  02 simple add/get addr                       [ OK ]
> >  03 dump addrs                                [ OK ]
> >  04 simple del addr                           [ OK ]
> >  05 dump addrs after del                      [ OK ]
> >  06 duplicate addr                            [ OK ]
> >  07 id addr increment                         [ OK ]
> >  08 hard addr limit                           [ OK ]
> >  09 above hard addr limit                     [ OK ]
> > 
> > simult_flows.sh
> >  01 balanced bwidth                          7411 max 8456       [
> > OK ]
> >  02 balanced bwidth - reverse direction      7380 max 8456       [
> > OK ]
> >  03 balanced bwidth with unbalanced delay    7434 max 8456       [
> > OK ]
> > 
> > userspace_pm.sh
> >  INFO: Init
> >  01 Created network namespaces ns1, ns2                       [ OK
> > ]
> >  INFO: Make connections
> >  02 Established IPv4 MPTCP Connection ns2 => ns1              [ OK
> > ]
> >  03 Established IPv6 MPTCP Connection ns2 => ns1              [ OK
> > ]
> >  INFO: Announce tests
> >  04 ADD_ADDR 10.0.2.2 (ns2) => ns1, invalid token             [ OK
> > ]
> >  05 ADD_ADDR id:14 10.0.2.2 (ns2) => ns1, reuse port          [ OK
> > ]
> > 
> > Having test counters helps to quickly identify issues when looking
> > at a
> > long list of output logs and results.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/diag.sh     |  8 ++---
> >  .../selftests/net/mptcp/mptcp_connect.sh      | 33 +++++++++++----
> > ----
> >  .../testing/selftests/net/mptcp/mptcp_lib.sh  |  8 +++++
> >  .../selftests/net/mptcp/mptcp_sockopt.sh      | 12 ++++---
> >  .../testing/selftests/net/mptcp/pm_netlink.sh |  6 ++--
> >  .../selftests/net/mptcp/simult_flows.sh       |  7 ++--
> >  .../selftests/net/mptcp/userspace_pm.sh       |  3 +-
> >  7 files changed, 47 insertions(+), 30 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/diag.sh
> > b/tools/testing/selftests/net/mptcp/diag.sh
> > index f9f62a8f41e3..01e9f11f1f47 100755
> > --- a/tools/testing/selftests/net/mptcp/diag.sh
> > +++ b/tools/testing/selftests/net/mptcp/diag.sh
> > @@ -9,7 +9,7 @@
> >  . "$(dirname "${0}")/mptcp_lib.sh"
> >  
> >  ns=""
> > -test_cnt=1
> > +test_cnt=0
> 
> Is it normal shellcheck doesn't complain about it not being used?

test_cnt still be used in this script in another place:

	ret=$test_cnt

> 
> >  timeout_poll=30
> >  timeout_test=$((timeout_poll * 2 + 1))
> >  ret=0
> > @@ -55,7 +55,7 @@ __chk_nr()
> >  
> >  	nr=$(eval $command)
> >  
> > -	printf "%-50s" "$msg"
> > +	mptcp_lib_print_test_counter test_cnt "%-50s" "$msg"
> 
> Same here, probably best with a helper, to avoid repeating the long
> list
> of arguments:
> 
>   # $1: test name
>   print_test() {
>       mptcp_lib_print_test_counter test_cnt "%-50s" "${1}"
>   }
> 
>   (...)
> 
>   print_test "${msg}"


print_title is added here.

> 
> Same in the other files not using a helper already (like
> mptcp_join.sh
> and userspace_pm.sh)
> 
> (see my comment in mptcp_lib.sh: maybe easier to remove params, and
> just
> call 'mptcp_lib_print_test_counter "${msg}"')
> 
> >  	if [ "$nr" != "$expected" ]; then
> >  		if [ "$nr" = "$skip" ] && !
> > mptcp_lib_expect_all_features; then
> >  			mptcp_lib_print_warn "[SKIP] Feature
> > probably not supported"
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index 06e945914ace..00bbe451e50d 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -131,6 +131,7 @@ ns2=""
> >  ns3=""
> >  ns4=""
> >  
> > +#shellcheck disable=SC2034
> 
> Please justify all disabled shellcheck checks, e.g.
> 
> #shellcheck disable=SC2034 # TEST_COUNT is used by mptcp_lib.sh

Updated.

> 
> same below
> 
> (see my comment below: why not defining it in mptcp_lib.sh then?)
> 
> >  TEST_COUNT=0
> >  TEST_GROUP=""
> >  
> > @@ -255,8 +256,9 @@ 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 -n -e "net.mptcp.enabled sysctl is not 1 by
> > default"
> > -		mptcp_lib_print_err "\t\t\t   [FAIL]"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s" \
> > +			"net.mptcp.enabled sysctl is not 1 by
> > default"
> 
> The test name should be printed before the test: so the counter will
> be
> incremented in case of issue or not:
> 
>   mptcp_lib_print_test_counter "MPTCP socket can be blocked via
> sysctl"
>   if [ ... ]; then
>        mptcp_lib_print_fail "net.mptcp.enabled sysctl is not (...)"
>        (...)
>   fi
> 
>   if [ ... ]; then
>        mptcp_lib_print_fail "MPTCP socket cannot be blocked via
> sysctl"
>        (...)
>   fi
> 
>   mptcp_lib_print_success

Updated.

> 
> 
> > +		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is
> > not 1 by default"
> >  		ret=1
> >  		return 1
> > @@ -269,15 +271,17 @@ check_mptcp_disabled()
> >  	mptcp_lib_ns_exit "${disabled_ns}"
> >  
> >  	if [ ${err} -eq 0 ]; then
> > -		echo -n -e "New MPTCP socket cannot be blocked via
> > sysctl"
> > -		mptcp_lib_print_err "\t\t\t   [FAIL]"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s" \
> > +			"New MPTCP socket cannot be blocked via
> > sysctl"
> > +		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "New MPTCP socket cannot be
> > blocked via sysctl"
> >  		ret=1
> >  		return 1
> >  	fi
> >  
> > -	echo -n -e "New MPTCP socket can be blocked via sysctl"
> > -	mptcp_lib_print_ok "\t\t\t   [ OK ]"
> > +	mptcp_lib_print_test_counter TEST_COUNT "%s" \
> > +		"New MPTCP socket can be blocked via sysctl"
> > +	mptcp_lib_print_ok "\t\t\t\t   [ OK ]"
> >  	mptcp_lib_result_pass "New MPTCP socket can be blocked via
> > sysctl"
> >  	return 0
> >  }
> > @@ -319,7 +323,6 @@ do_transfer()
> >  
> >  	local port
> >  	port=$((10000+PORT++))
> > -	TEST_COUNT=$((TEST_COUNT+1))
> >  
> >  	if [ "$rcvbuf" -gt 0 ]; then
> >  		extra_args="$extra_args -R $rcvbuf"
> > @@ -346,7 +349,7 @@ do_transfer()
> >  	addr_port=$(printf "%s:%d" ${connect_addr} ${port})
> >  	local result_msg
> >  	result_msg="$(printf "%.3s %-5s -> %.3s (%-20s) %-5s"
> > ${connector_ns} ${cl_proto} ${listener_ns} ${addr_port}
> > ${srv_proto})"
> > -	printf "%s\t" "${result_msg}"
> > +	mptcp_lib_print_test_counter TEST_COUNT "%s\t"
> > "${result_msg}"
> >  
> >  	if $capture; then
> >  		local capuser
> > @@ -663,7 +666,8 @@ run_test_transparent()
> >  	# following function has been exported (T). Not great but
> > better than
> >  	# checking for a specific kernel version.
> >  	if ! mptcp_lib_kallsyms_has "T __ip_sock_set_tos$"; then
> > -		echo "INFO: ${msg} not supported by the kernel:
> > SKIP"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"INFO: ${msg} not supported by the kernel:
> > SKIP"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> >  	fi
> > @@ -680,7 +684,8 @@ table inet mangle {
> >  }
> >  EOF
> >  	then
> > -		echo "SKIP: $msg, could not load nft ruleset"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"SKIP: $msg, could not load nft ruleset"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_fail_if_expected_feature "nft rules"
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> > @@ -696,7 +701,8 @@ EOF
> >  
> >  	if ! ip -net "$listener_ns" $r6flag rule add fwmark 1
> > lookup 100; then
> >  		ip netns exec "$listener_ns" nft flush ruleset
> > -		echo "SKIP: $msg, ip $r6flag rule failed"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"SKIP: $msg, ip $r6flag rule failed"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_fail_if_expected_feature "ip rule"
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> > @@ -705,7 +711,8 @@ EOF
> >  	if ! ip -net "$listener_ns" route add local $local_addr/0
> > dev lo table 100; then
> >  		ip netns exec "$listener_ns" nft flush ruleset
> >  		ip -net "$listener_ns" $r6flag rule del fwmark 1
> > lookup 100
> > -		echo "SKIP: $msg, ip route add local $local_addr
> > failed"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"SKIP: $msg, ip route add local
> > $local_addr failed"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_fail_if_expected_feature "ip route"
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> > @@ -861,7 +868,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
> >  
> >  stop_if_error "Could not even run ping tests"
> >  
> > -echo -e "ping tests"
> > +mptcp_lib_print_test_counter TEST_COUNT "%s" "ping tests"
> >  mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"
> >  
> >  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root
> > netem loss random $tc_loss delay ${tc_delay}ms
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > index 7e309493eda2..df495658f043 100644
> > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > @@ -411,3 +411,11 @@ mptcp_lib_events() {
> >  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1
> > &
> >  	pid=$!
> >  }
> > +
> > +mptcp_lib_print_test_counter() {
> 
> Maybe more "mptcp_lib_print_test_title" or "_name": it doesn't just
> print the counter, mostly the "title" (including the counter, part of
> the title)

Rename to mptcp_lib_print_title.

> 
> > +	declare -n counter="${1}"
> > +	local fmt="${2}"
> > +	local msg="${3}"
> > +
> > +	printf "%02u ${fmt}" "$((++counter))" "${msg}"
> 
> Maybe having this:
> 
>   : "${MPTCP_LIB_PRINT_TEST_FORMAT:="%02u %-50s"}"
>   MPTCP_LIB_TEST_COUNTER=0
> 
>   (...)
> 
>   # $1: test name
>   mptcp_lib_print_test_counter() {
>       printf "${MPTCP_LIB_PRINT_TEST_FORMAT}" \
>              "$((++MPTCP_LIB_TEST_COUNTER))" "${1}"
>   }
> 
> Would simplify stuff?

Do you mean to define TEST_COUNT in mptcp_lib.sh? That means we need to
rename test_cnt to TEST_COUNT. I remember you objected to move vars
into mptcp_lib.sh like in "selftests: mptcp: add mptcp_lib_ns_init/exit
helpers". So I pass TEST_COUNT or test_cnt as an argument to
mptcp_lib_print_test_counter().

I think this can keep continuity with other functions in mptcp_lib,
like mptcp_lib_ns_init/exit.

> 
> So tests can specify the format by setting
> MPTCP_LIB_PRINT_TEST_FORMAT,
> e.g. to support more than 99 entries. And you don't need to define
> TEST_COUNT and disable SC2034.
> 
> Would that work?
> 
> You could even use it in mptcp_join.sh: print_title() would call this
> new helper.
> 
> > +}
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > index cfa0cfb918f4..a2a5049f22bb 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > @@ -17,6 +17,8 @@ timeout_poll=30
> >  timeout_test=$((timeout_poll * 2 + 1))
> >  iptables="iptables"
> >  ip6tables="ip6tables"
> > +#shellcheck disable=SC2034
> > +test_cnt=0
> >  
> >  ns1=""
> >  ns2=""
> > @@ -161,7 +163,7 @@ do_transfer()
> >  	wait $spid
> >  	local rets=$?
> >  
> > -	printf "%-50s" "transfer ${ip}"
> > +	mptcp_lib_print_test_counter test_cnt "%-50s" "transfer
> > ${ip}"
> 
> Same my comments on previous patches: if here a helper is used to
> print
> the test name, you would only need to change this helper, not doing
> the
> same modification multiple times at the same places.

Do this in print_title helper.

> 
> >  	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
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh
> > b/tools/testing/selftests/net/mptcp/simult_flows.sh
> > index 79cb377ee0bd..eb2eaa48035f 100755
> > --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> > +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> > @@ -14,7 +14,7 @@ ns3=""
> >  capture=false
> >  timeout_poll=30
> >  timeout_test=$((timeout_poll * 2 + 1))
> > -test_cnt=1
> > +test_cnt=0
> 
> Why ShellCheck is not complaining the variable is not used here?

test_cnt still be used in this script.

> 
> >  ret=0
> >  bail=0
> >  slack=50
> 
> (...)
> 
> While at it, can you make sure all the results are aligned for
> simult_flows by increasing the '%-XXs'?
> 
> > diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > index 33bbb0d5807f..27f308601005 100755
> > --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > @@ -53,6 +53,7 @@ server_addr_id=${RANDOM:0:2}
> >  ns1=""
> >  ns2=""
> >  ret=0
> 
> Why ShellCheck is not complaining the variable is not used here?

The same.

> 
> > +test_cnt=0
> >  test_name=""
> >  
> >  _printf() {
> 
> (...)
> 
> Cheers,
> Matt