Re: [PATCH mptcp-next v7 6/8] selftests: mptcp: add test counter helpers

[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 two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
> the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
> Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
> counter, and mptcp_lib_pr_title_counter(), print the test title with
> counter. They are used in mptcp_join.sh first.

Please add the reason, something like: Each MPTCP selftest is having
subtests, and it helps to give them a number to quickly identify them.
This can be managed by mptcp_lib.sh, reusing what has been done here.
The following commit will use these new helpers in the other tests.

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 16 ++++++++--------
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh  | 17 +++++++++++++++++
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 1df2d24979a0..7acc6064eb17 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -48,7 +48,6 @@ declare -A all_tests
>  declare -a only_tests_ids
>  declare -a only_tests_names
>  declare -A failed_tests
> -TEST_COUNT=0
>  TEST_NAME=""
>  nr_blank=6
>  
> @@ -172,7 +171,8 @@ cleanup()
>  
>  print_title()
>  {
> -	printf "%03u %s\n" "${TEST_COUNT}" "${TEST_NAME}"
> +	MPTCP_LIB_TEST_FORMAT="%03u %s\n"

No need to re-set it all the time, this line can be moved just before
"sourcing" mptcp_lib.sh I think, with a comment like:

  # Here, we have more than 100 tests, having multiple checks
  MPTCP_LIB_TEST_FORMAT="%03u %s\n"

  . "$(dirname "${0}")/mptcp_lib.sh"

> +	mptcp_lib_pr_title_counter "${TEST_NAME}"
>  }
>  
>  print_check()

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 763a2989ca6d..cbf0dd2cc4cb 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -411,3 +411,20 @@ mptcp_lib_events() {
>  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
>  	pid=$!
>  }
> +
> +MPTCP_LIB_TEST_COUNTER=0
> +MPTCP_LIB_TEST_FORMAT=""

Can you declare them at the top, around the other ones?
(MPTCP_LIB_SUBTESTS, etc.)

And I think you should set the default value there too, like what I was
suggesting in patch 07/12 from v5:

  : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
  MPTCP_LIB_TEST_COUNTER=0

So we can change the format by setting MPTCP_LIB_TEST_FORMAT before
"sourcing" mptcp_lib.sh.

No?

*If* we need to alter the format just for one title, we can always do:

  MPTCP_LIB_TEST_FORMAT="..." \
      mptcp_lib_pr_title_counter "<title>"

But best to avoid that.

> +
> +mptcp_lib_inc_test_counter() {
> +	: "${MPTCP_LIB_TEST_COUNTER:?}"

I don't think that's needed, it has been defined in the file, no?

> +
> +	MPTCP_LIB_TEST_COUNTER=$((MPTCP_LIB_TEST_COUNTER+1))
> +}
> +
> +#shellcheck disable=SC2059

Please always justify why you need to ignore a rule.

Also, always try to reduce the scope as much as possible: just for one
line, for one function, for the whole file → here, move it just before
calling 'printf':

  # shellcheck disable=SC2059 # the format is in a variable
  printf "${MPTCP_LIB_TEST_FORMAT}" (...)

(and add a space after '#')

> +mptcp_lib_pr_title_counter() {
> +	: "${MPTCP_LIB_TEST_COUNTER:?}"
> +	: "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
> +
> +	printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"

I didn't check: can you not print the title and increment the counter
(via mptcp_lib_inc_test_counter) from here? I think that would be a good
practice to do that instead of having to deal with the counter in
different places.

We would only call "mptcp_lib_inc_test_counter()" from other scripts
when a test is skipped.

> +}

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