From: Geliang Tang <tanggeliang@kylinos.cn>

Extract the main part of check_expected() in userspace_pm.sh to a new
function mptcp_lib_check_expected() in mptcp_lib.sh. It will be used
in both mptcp_john.sh and userspace_pm.sh. check_expected_one() is
moved into mptcp_lib.sh too as mptcp_lib_check_expected_one().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 30 ++++++++++++++++
 .../selftests/net/mptcp/userspace_pm.sh       | 34 +++----------------
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index ea39392c68e7..44491f18ed17 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -438,3 +438,33 @@ mptcp_lib_print_title() {
 	# shellcheck disable=SC2059 # the format is in a variable
 	printf "${MPTCP_LIB_TEST_FORMAT}" "$((++MPTCP_LIB_TEST_COUNTER))" "${*}"
 }
+
+# $1: var name ; $2: prev ret
+mptcp_lib_check_expected_one() {
+	local var="${1}"
+	local exp="e_${var}"
+	local prev_ret="${2}"
+
+	if [ "${!var}" = "${!exp}" ]; then
+		return 0
+	fi
+
+	if [ "${prev_ret}" = "0" ]; then
+		mptcp_lib_pr_fail
+	fi
+
+	mptcp_lib_print_err "Expected value for '${var}': '${!exp}', got '${!var}'."
+	return 1
+}
+
+# $@: all var names to check
+mptcp_lib_check_expected() {
+	local rc=0
+	local var
+
+	for var in "${@}"; do
+		mptcp_lib_check_expected_one "${var}" "${rc}" || rc=1
+	done
+
+	return "${rc}"
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 4e29aa9c2529..6ab9efda8627 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -5,7 +5,7 @@
 # code but we accept it.
 #shellcheck disable=SC2086
 
-# Some variables are used below but indirectly, see check_expected_one()
+# Some variables are used below but indirectly, see verify_*_event()
 #shellcheck disable=SC2034
 
 . "$(dirname "${0}")/mptcp_lib.sh"
@@ -225,44 +225,20 @@ make_connection()
 	fi
 }
 
-# $1: var name ; $2: prev ret
-check_expected_one()
-{
-	local var="${1}"
-	local exp="e_${var}"
-	local prev_ret="${2}"
-
-	if [ "${!var}" = "${!exp}" ]
-	then
-		return 0
-	fi
-
-	if [ "${prev_ret}" = "0" ]
-	then
-		test_fail
-	fi
-
-	mptcp_lib_print_err "\tExpected value for '${var}': '${!exp}', got '${!var}'."
-	return 1
-}
-
 # $@: all var names to check
 check_expected()
 {
 	local rc=0
-	local var
 
-	for var in "${@}"
-	do
-		check_expected_one "${var}" "${rc}" || rc=1
-	done
-
-	if [ ${rc} -eq 0 ]
+	mptcp_lib_check_expected "${@}" || rc=1
+	if [ "${rc}" -eq 0 ]
 	then
 		test_pass
 		return 0
 	fi
 
+	ret=1
+	mptcp_lib_result_fail "${test_name}"
 	return 1
 }
 
-- 
2.40.1

Hi Geliang,

On 07/03/2024 13:48, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Extract the main part of check_expected() in userspace_pm.sh to a new
> function mptcp_lib_check_expected() in mptcp_lib.sh. It will be used
> in both mptcp_john.sh and userspace_pm.sh. check_expected_one() is
> moved into mptcp_lib.sh too as mptcp_lib_check_expected_one().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 30 ++++++++++++++++
>  .../selftests/net/mptcp/userspace_pm.sh       | 34 +++----------------
>  2 files changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index ea39392c68e7..44491f18ed17 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -438,3 +438,33 @@ mptcp_lib_print_title() {
>  	# shellcheck disable=SC2059 # the format is in a variable
>  	printf "${MPTCP_LIB_TEST_FORMAT}" "$((++MPTCP_LIB_TEST_COUNTER))" "${*}"
>  }
> +
> +# $1: var name ; $2: prev ret
> +mptcp_lib_check_expected_one() {
> +	local var="${1}"
> +	local exp="e_${var}"
> +	local prev_ret="${2}"
> +
> +	if [ "${!var}" = "${!exp}" ]; then
> +		return 0
> +	fi
> +
> +	if [ "${prev_ret}" = "0" ]; then
> +		mptcp_lib_pr_fail
> +	fi
> +
> +	mptcp_lib_print_err "Expected value for '${var}': '${!exp}', got '${!var}'."

Maybe keep the tab like before? Please check how it looks like when
there is a failure in mptcp_join.sh and userspace_pm.sh.

> +	return 1
> +}
> +
> +# $@: all var names to check
> +mptcp_lib_check_expected() {
> +	local rc=0
> +	local var
> +
> +	for var in "${@}"; do
> +		mptcp_lib_check_expected_one "${var}" "${rc}" || rc=1
> +	done
> +
> +	return "${rc}"
> +}
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 4e29aa9c2529..6ab9efda8627 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -5,7 +5,7 @@
>  # code but we accept it.
>  #shellcheck disable=SC2086
>  
> -# Some variables are used below but indirectly, see check_expected_one()
> +# Some variables are used below but indirectly, see verify_*_event()
>  #shellcheck disable=SC2034
>  
>  . "$(dirname "${0}")/mptcp_lib.sh"
> @@ -225,44 +225,20 @@ make_connection()
>  	fi
>  }
>  
> -# $1: var name ; $2: prev ret
> -check_expected_one()
> -{
> -	local var="${1}"
> -	local exp="e_${var}"
> -	local prev_ret="${2}"
> -
> -	if [ "${!var}" = "${!exp}" ]
> -	then
> -		return 0
> -	fi
> -
> -	if [ "${prev_ret}" = "0" ]
> -	then
> -		test_fail
> -	fi
> -
> -	mptcp_lib_print_err "\tExpected value for '${var}': '${!exp}', got '${!var}'."
> -	return 1
> -}
> -
>  # $@: all var names to check
>  check_expected()
>  {
>  	local rc=0
> -	local var
>  
> -	for var in "${@}"
> -	do
> -		check_expected_one "${var}" "${rc}" || rc=1
> -	done
> -
> -	if [ ${rc} -eq 0 ]
> +	mptcp_lib_check_expected "${@}" || rc=1
> +	if [ "${rc}" -eq 0 ]

I guess you no longer need "rc":

  if mptcp_lib_check_expected "${@}"
  then
      test_pass
      return 0
  fi

>  	then
>  		test_pass
>  		return 0
>  	fi
>  
> +	ret=1
> +	mptcp_lib_result_fail "${test_name}"

Mmh, you should keep using test_fail(): best to keep using a single
helper in case of failure, because we need to do these two actions, and
for sure, we will miss one later in new code by accident.

Instead, I suggest modifying test_fail() to call "mptcp_lib_pr_fail"
only if there are arguments (if [ ${#} -gt 0 ]). But then, you will have
to add arguments when calling test_fail() from test_remove() → e.g.
"unexpected type: ${type}" (it even makes sense to print that any way to
help to understand what's wrong)

>  	return 1
>  }
>  

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