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

[Matthieu Baerts <matttbe@kernel.org>]

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To unify the output formats of all test scripts, this patch adds
> four more helpers:
> 
> 	mptcp_lib_pr_ok()
> 	mptcp_lib_pr_skip()
> 	mptcp_lib_pr_fail()
> 	mptcp_lib_pr_info()
> 
> to print out [ OK ], [SKIP], [FAIL] and 'INFO: ' with colors. Use them
> in all scripts to print the "ok/skip/fail/info' using the same 'format'.
> 
> Having colors helps to quickly identify issues when looking at a long
> list of output logs and results.
> 
> Note that the mptcp_join.sh tests will now print these keywords with
> capital letters, like most of the other tests.

I think it is not just mptcp_join.sh that was using different keywords.

  Note that now all print the same keywords, which was not the case
  before, but it is good to uniform that.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 096ff8941c5b..70acbb19f568 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh

(...)

> @@ -534,7 +534,6 @@ do_transfer()
>  			"${expect_ackrx}" "${stat_ackrx_now_l}"
>  	fi
>  
> -	echo

Just to be sure: do we still have something good in case of error?

I mean: why were we not printing a new line before? Maybe there was no
reason, or maybe sometimes we print more details? (syn cookies?)

Maybe that's fine with the new output, it is just to know if you checked
that.

>  	cat "$capout"
>  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
>  }

(...)

> @@ -810,7 +809,7 @@ log_if_error()
>  	local msg="$1"
>  
>  	if [ ${ret} -ne 0 ]; then
> -		echo "FAIL: ${msg}" 1>&2
> +		mptcp_lib_pr_fail "${msg}"

Good to mention in the commit message that all errors messages are
printed to 'stdout' now, as part of the uniformation. (or do that in an
dedicated commit)

>  
>  		final_ret=${ret}
>  		ret=0
> @@ -858,11 +857,11 @@ done
>  mptcp_lib_result_code "${ret}" "ping tests"
>  
>  stop_if_error "Could not even run ping tests"
> -echo "[ OK ]"
> +mptcp_lib_pr_ok
>  MPTCP_LIB_TEST_FORMAT="%02u %-50s"
>  
>  [ -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 "

Here, it was using 'echo -n' ...

> +mptcp_lib_pr_info "Using loss of $tc_loss "

Now a new line will be printed at the end (with a trailing whitespace)

>  test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "

And this will be printed in a new line, it will look strange I think

Maybe use a variable to store all the info?

  tc_info="loss of $tc_loss "
  test "$tc_delay" -gt 0 && tc_info+="delay $tc_delay ms "
  (...)
  mptcp_lib_pr_info "Using ${tc_info}"

>  
>  reorder_delay=$((tc_delay / 4))

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 358d5b77fc0f..53867f9d212d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -48,6 +48,23 @@ mptcp_lib_print_err() {
>  	mptcp_lib_print_color "${MPTCP_LIB_COLOR_RED}${*}"
>  }
>  
> +#shellcheck disable=SC2120

Add a space after '#' and add a comment justifying the exception, like
the one I shared in patch 1/9 from v6:

  # shellcheck disable=SC2120  # parameters are optional

Out of curiosity, we don't need this for 'skip' and 'fail'?

> +mptcp_lib_pr_ok() {
> +	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
> +}
> +
> +mptcp_lib_pr_skip() {
> +	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
> +}
> +
> +mptcp_lib_pr_fail() {
> +	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
> +}
> +
> +mptcp_lib_pr_info() {
> +	mptcp_lib_print_info "INFO: ${*}"
> +}
> +
>  # SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var can be set when validating all
>  # features using the last version of the kernel and the selftests to make sure
>  # a test is not being skipped by mistake.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index a797e13d3fe7..d695561fbf46 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh

(...)

> @@ -217,7 +218,7 @@ do_mptcp_sockopt_tests()
>  	local lret=0
>  
>  	if ! mptcp_lib_kallsyms_has "mptcp_diag_fill_info$"; then
> -		echo "INFO: MPTCP sockopt not supported: SKIP"
> +		mptcp_lib_pr_skip "MPTCP sockopt not supported"
>  		mptcp_lib_result_skip "sockopt"
>  		return
>  	fi
> @@ -227,12 +228,12 @@ do_mptcp_sockopt_tests()
>  
>  	mptcp_lib_print_title "SOL_MPTCP sockopt v4"
>  	if [ $lret -ne 0 ]; then
> -		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
> +		mptcp_lib_pr_fail "SOL_MPTCP getsockopt"

No need to repeat what is already in the title. (I didn't check them
all, but I guess some other subtests here repeat stuff already in the
title, e.g. the same for v6)

>  		mptcp_lib_result_fail "sockopt v4"
>  		ret=$lret
>  		return
>  	fi
> -	echo "[ OK ]"
> +	mptcp_lib_pr_ok
>  	mptcp_lib_result_pass "sockopt v4"
>  
>  	ip netns exec "$ns_sbox" ./mptcp_sockopt -6

(...)

> @@ -288,7 +289,7 @@ do_tcpinq_tests()
>  	local lret=0
>  
>  	if ! mptcp_lib_kallsyms_has "mptcp_ioctl$"; then
> -		echo "INFO: TCP_INQ not supported: SKIP"
> +		mptcp_lib_pr_skip "INFO: TCP_INQ not supported"

You can remove 'INFO: ' I  suppose.

>  		mptcp_lib_result_skip "TCP_INQ"
>  		return
>  	fi

(...)

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