[PATCH mptcp-net 0/5] selftests: mptcp: unique subtest names

[PATCH mptcp-net 0/5] selftests: mptcp: unique subtest names

[Matthieu Baerts (NGI0)]

It is important to have a unique (sub)test name in TAP, because some CI
environments drop tests with duplicated name.

When working on adding subtests support in Netdev's NIPA CI, I found out
some subtests had the same name. The name is used as a key, it should
then be unique, otherwise we don't track duplicated tests.

The fixes have been split in different patches, because it fixes
different commits. But the idea is always the same: make sure subtest
names are unique.

The last patch is not for -net, that's a new feature to make sure we
don't add duplicated names later.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (5):
      selftests: mptcp: simult flows: fix some subtest names
      selftests: mptcp: userspace_pm: unique subtest names
      selftests: mptcp: diag: unique 'in use' subtest names
      selftests: mptcp: diag: unique 'cestab' subtest names
      selftests: mptcp: lib: catch duplicated subtest entries

 tools/testing/selftests/net/mptcp/diag.sh         | 37 ++++++++++++++---------
 tools/testing/selftests/net/mptcp/mptcp_lib.sh    | 21 +++++++++++++
 tools/testing/selftests/net/mptcp/simult_flows.sh |  3 +-
 tools/testing/selftests/net/mptcp/userspace_pm.sh |  4 +--
 4 files changed, 48 insertions(+), 17 deletions(-)
---
base-commit: 0c8d1475f726a95cddb393e45f0b4d8ec0e15785
change-id: 20240209-selftests-mptcp-uniq-title-2e2df0c17695

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net 1/5] selftests: mptcp: simult flows: fix some subtest names

[Matthieu Baerts (NGI0)]

The selftest was correctly recording all the results, but the 'reverse
direction' part was missing in the name when needed.

It is important to have a unique (sub)test name in TAP, because some CI
environments drop tests with duplicated name.

Fixes: 675d99338e7a ("selftests: mptcp: simult flows: format subtests results in TAP")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/simult_flows.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 619be0e1acf5..f377ef01970b 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -250,7 +250,8 @@ run_test()
 		[ $bail -eq 0 ] || exit $ret
 	fi
 
-	printf "%-60s" "$msg - reverse direction"
+	msg="${msg} - reverse direction"
+	printf "%-60s" "${msg}"
 	do_transfer $large $small $time
 	lret=$?
 	mptcp_lib_result_code "${lret}" "${msg}"

-- 
2.43.0

[PATCH mptcp-net 2/5] selftests: mptcp: userspace_pm: unique subtest names

[Matthieu Baerts (NGI0)]

It is important to have a unique (sub)test name in TAP, because some CI
environments drop tests with duplicated names.

Some subtests from the userspace_pm selftest had the same names. That's
because different subflows are created (and deleted) between the same
pair of IP addresses.

Simply adding the destination port in the name is then enough to have
different names, because the destination port is always different.

Note that adding such info takes a bit more space, so we need to
increase a bit the width to print the name, simply to keep all the
'[ OK ]' aligned as before.

Fixes: f589234e1af0 ("selftests: mptcp: userspace_pm: format subtests results in TAP")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 6167837f48e1..1b94a75604fe 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -75,7 +75,7 @@ print_test()
 {
 	test_name="${1}"
 
-	_printf "%-63s" "${test_name}"
+	_printf "%-68s" "${test_name}"
 }
 
 print_results()
@@ -542,7 +542,7 @@ verify_subflow_events()
 	local remid
 	local info
 
-	info="${e_saddr} (${e_from}) => ${e_daddr} (${e_to})"
+	info="${e_saddr} (${e_from}) => ${e_daddr}:${e_dport} (${e_to})"
 
 	if [ "$e_type" = "$SUB_ESTABLISHED" ]
 	then

-- 
2.43.0

[PATCH mptcp-net 3/5] selftests: mptcp: diag: unique 'in use' subtest names

[Matthieu Baerts (NGI0)]

It is important to have a unique (sub)test name in TAP, because some CI
environments drop tests with duplicated name.

Some 'in use' subtests from the diag selftest had the same names, e.g.:

    chk 0 msk in use after flush

Now the previous value is taken, to have different names, e.g.:

    chk 2->0 msk in use after flush

While at it, avoid repeating the full message, declare it once in the
helper.

Fixes: ce9902573652 ("selftests: mptcp: diag: format subtests results in TAP")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/diag.sh | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 04fcb8a077c9..7e7e40601c6b 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -166,9 +166,13 @@ chk_msk_listen()
 chk_msk_inuse()
 {
 	local expected=$1
-	local msg="$2"
+	local msg="....chk ${2:-${expected}} msk in use"
 	local listen_nr
 
+	if [ "${expected}" -eq 0 ]; then
+		msg="${msg} after flush"
+	fi
+
 	listen_nr=$(ss -N "${ns}" -Ml | grep -c LISTEN)
 	expected=$((expected + listen_nr))
 
@@ -179,7 +183,7 @@ chk_msk_inuse()
 		sleep 0.1
 	done
 
-	__chk_nr get_msk_inuse $expected "$msg" 0
+	__chk_nr get_msk_inuse $expected "${msg}" 0
 }
 
 # $1: cestab nr
@@ -227,11 +231,11 @@ wait_connected $ns 10000
 chk_msk_nr 2 "after MPC handshake "
 chk_msk_remote_key_nr 2 "....chk remote_key"
 chk_msk_fallback_nr 0 "....chk no fallback"
-chk_msk_inuse 2 "....chk 2 msk in use"
+chk_msk_inuse 2
 chk_msk_cestab 2
 flush_pids
 
-chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_inuse 0 "2->0"
 chk_msk_cestab 0
 
 echo "a" | \
@@ -247,11 +251,11 @@ echo "b" | \
 				127.0.0.1 >/dev/null &
 wait_connected $ns 10001
 chk_msk_fallback_nr 1 "check fallback"
-chk_msk_inuse 1 "....chk 1 msk in use"
+chk_msk_inuse 1
 chk_msk_cestab 1
 flush_pids
 
-chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_inuse 0 "1->0"
 chk_msk_cestab 0
 
 NR_CLIENTS=100
@@ -273,11 +277,11 @@ for I in `seq 1 $NR_CLIENTS`; do
 done
 
 wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
-chk_msk_inuse $((NR_CLIENTS*2)) "....chk many msk in use"
+chk_msk_inuse $((NR_CLIENTS*2)) "many"
 chk_msk_cestab $((NR_CLIENTS*2))
 flush_pids
 
-chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_inuse 0 "many->0"
 chk_msk_cestab 0
 
 mptcp_lib_result_print_all_tap

-- 
2.43.0

[PATCH mptcp-net 4/5] selftests: mptcp: diag: unique 'cestab' subtest names

[Matthieu Baerts (NGI0)]

It is important to have a unique (sub)test name in TAP, because some CI
environments drop tests with duplicated name.

Some 'cestab' subtests from the diag selftest had the same names, e.g.:

    ....chk 0 cestab

Now the previous value is taken, to have different names, e.g.:

    ....chk 2->0 cestab after flush

While at it, the 'after flush' info is added, similar to what is done
with the 'in use' subtests. Also inspired by these 'in use' subtests,
'many' is displayed instead of a large number:

    many msk socket present                           [  ok  ]
    ....chk many msk in use                           [  ok  ]
    ....chk many cestab                               [  ok  ]
    ....chk many->0 msk in use after flush            [  ok  ]
    ....chk many->0 cestab after flush                [  ok  ]

Fixes: 81ab772819da ("selftests: mptcp: diag: check CURRESTAB counters")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/diag.sh | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 7e7e40601c6b..2cffa85a0bb7 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -189,10 +189,15 @@ chk_msk_inuse()
 # $1: cestab nr
 chk_msk_cestab()
 {
-	local cestab=$1
+	local expected=$1
+	local msg="....chk ${2:-${expected}} cestab"
+
+	if [ "${expected}" -eq 0 ]; then
+		msg="${msg} after flush"
+	fi
 
 	__chk_nr "mptcp_lib_get_counter ${ns} MPTcpExtMPCurrEstab" \
-		 "${cestab}" "....chk ${cestab} cestab" ""
+		 "${expected}" "${msg}" ""
 }
 
 wait_connected()
@@ -236,7 +241,7 @@ chk_msk_cestab 2
 flush_pids
 
 chk_msk_inuse 0 "2->0"
-chk_msk_cestab 0
+chk_msk_cestab 0 "2->0"
 
 echo "a" | \
 	timeout ${timeout_test} \
@@ -256,7 +261,7 @@ chk_msk_cestab 1
 flush_pids
 
 chk_msk_inuse 0 "1->0"
-chk_msk_cestab 0
+chk_msk_cestab 0 "1->0"
 
 NR_CLIENTS=100
 for I in `seq 1 $NR_CLIENTS`; do
@@ -278,11 +283,11 @@ done
 
 wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
 chk_msk_inuse $((NR_CLIENTS*2)) "many"
-chk_msk_cestab $((NR_CLIENTS*2))
+chk_msk_cestab $((NR_CLIENTS*2)) "many"
 flush_pids
 
 chk_msk_inuse 0 "many->0"
-chk_msk_cestab 0
+chk_msk_cestab 0 "many->0"
 
 mptcp_lib_result_print_all_tap
 exit $ret

-- 
2.43.0

[PATCH mptcp-net 5/5] selftests: mptcp: lib: catch duplicated subtest entries

[Matthieu Baerts (NGI0)]

It is important to have a unique (sub)test name in TAP, because some CI
environments drop tests with duplicated name.

When adding a new subtest entry, an error message is printed in case of
duplicated entries. If there were duplicated entries and if all features
were expected to work, the script exits with an error at the end, after
having printed all subtests in the TAP format. Thanks to that, the MPTCP
CI will catch such issues early.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
  - This is a patch for 'mptcp-next'. But because it depends on the
    other ones, I added it in this series, also to check I didn't miss
    anything.
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 3a2abae5993e..037cb3e84330 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -9,6 +9,7 @@ readonly KSFT_SKIP=4
 readonly KSFT_TEST="${MPTCP_LIB_KSFT_TEST:-$(basename "${0}" .sh)}"
 
 MPTCP_LIB_SUBTESTS=()
+MPTCP_LIB_SUBTESTS_DUPLICATED=0
 
 # only if supported (or forced) and not disabled, see no-color.org
 if { [ -t 1 ] || [ "${SELFTESTS_MPTCP_LIB_COLOR_FORCE:-}" = "1" ]; } &&
@@ -146,12 +147,26 @@ mptcp_lib_kversion_ge() {
 	mptcp_lib_fail_if_expected_feature "kernel version ${1} lower than ${v}"
 }
 
+__mptcp_lib_result_check_duplicated() {
+	local subtest
+
+	for subtest in "${MPTCP_LIB_SUBTESTS[@]}"; do
+		if [[ "${subtest}" == *" - ${KSFT_TEST}: ${*%% #*}" ]]; then
+			MPTCP_LIB_SUBTESTS_DUPLICATED=1
+			mptcp_lib_print_err "Duplicated entry: ${*}"
+			break
+		fi
+	done
+}
+
 __mptcp_lib_result_add() {
 	local result="${1}"
 	shift
 
 	local id=$((${#MPTCP_LIB_SUBTESTS[@]} + 1))
 
+	__mptcp_lib_result_check_duplicated "${*}"
+
 	MPTCP_LIB_SUBTESTS+=("${result} ${id} - ${KSFT_TEST}: ${*}")
 }
 
@@ -206,6 +221,12 @@ mptcp_lib_result_print_all_tap() {
 	for subtest in "${MPTCP_LIB_SUBTESTS[@]}"; do
 		printf "%s\n" "${subtest}"
 	done
+
+	if [ "${MPTCP_LIB_SUBTESTS_DUPLICATED}" = 1 ] &&
+	   mptcp_lib_expect_all_features; then
+		mptcp_lib_print_err "Duplicated test entries"
+		exit ${KSFT_FAIL}
+	fi
 }
 
 # get the value of keyword $1 in the line marked by keyword $2

-- 
2.43.0

Re: selftests: mptcp: lib: catch duplicated subtest entries: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7847651579

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/27b6d52de8b3


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: selftests: mptcp: lib: catch duplicated subtest entries: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI (Cirrus) did some validations with a debug kernel and here is its report:

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_regressions 🔴:
  - Task: https://cirrus-ci.com/task/6628165980782592
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6628165980782592/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6733421603454976
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6733421603454976/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/27b6d52de8b3


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net 0/5] selftests: mptcp: unique subtest names

[Geliang Tang]

On Fri, Feb 09, 2024 at 06:28:38PM +0100, Matthieu Baerts (NGI0) wrote:
> It is important to have a unique (sub)test name in TAP, because some CI
> environments drop tests with duplicated name.
> 
> When working on adding subtests support in Netdev's NIPA CI, I found out
> some subtests had the same name. The name is used as a key, it should
> then be unique, otherwise we don't track duplicated tests.
> 
> The fixes have been split in different patches, because it fixes
> different commits. But the idea is always the same: make sure subtest
> names are unique.
> 
> The last patch is not for -net, that's a new feature to make sure we
> don't add duplicated names later.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (5):
>       selftests: mptcp: simult flows: fix some subtest names
>       selftests: mptcp: userspace_pm: unique subtest names
>       selftests: mptcp: diag: unique 'in use' subtest names
>       selftests: mptcp: diag: unique 'cestab' subtest names
>       selftests: mptcp: lib: catch duplicated subtest entries

Hi Matt,

I'd like to review this series this week. Please delegate them for
me on Patchwork. I can't delegate them by myself.

Thanks,
-Geliang

> 
>  tools/testing/selftests/net/mptcp/diag.sh         | 37 ++++++++++++++---------
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh    | 21 +++++++++++++
>  tools/testing/selftests/net/mptcp/simult_flows.sh |  3 +-
>  tools/testing/selftests/net/mptcp/userspace_pm.sh |  4 +--
>  4 files changed, 48 insertions(+), 17 deletions(-)
> ---
> base-commit: 0c8d1475f726a95cddb393e45f0b4d8ec0e15785
> change-id: 20240209-selftests-mptcp-uniq-title-2e2df0c17695
> 
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 

Re: [PATCH mptcp-net 1/5] selftests: mptcp: simult flows: fix some subtest names

[Geliang Tang]

Hi Matt,

On Fri, Feb 09, 2024 at 06:28:39PM +0100, Matthieu Baerts (NGI0) wrote:
> The selftest was correctly recording all the results, but the 'reverse
> direction' part was missing in the name when needed.
> 
> It is important to have a unique (sub)test name in TAP, because some CI
> environments drop tests with duplicated name.
> 
> Fixes: 675d99338e7a ("selftests: mptcp: simult flows: format subtests results in TAP")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/simult_flows.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index 619be0e1acf5..f377ef01970b 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -250,7 +250,8 @@ run_test()
>  		[ $bail -eq 0 ] || exit $ret
>  	fi
>  
> -	printf "%-60s" "$msg - reverse direction"
> +	msg="${msg} - reverse direction"

How about using '+=' operator to append a string like:

	msg+=" - reverse direction"

The same in patch 3 and patch 4:

	msg+=" after flush"

WDYT?

Thanks,
-Geliang

> +	printf "%-60s" "${msg}"
>  	do_transfer $large $small $time
>  	lret=$?
>  	mptcp_lib_result_code "${lret}" "${msg}"
> 
> -- 
> 2.43.0
> 

Re: [PATCH mptcp-net 0/5] selftests: mptcp: unique subtest names

[Matthieu Baerts]

Hi Geliang,

On 13/02/2024 05:21, Geliang Tang wrote:
> On Fri, Feb 09, 2024 at 06:28:38PM +0100, Matthieu Baerts (NGI0) wrote:
>> It is important to have a unique (sub)test name in TAP, because some CI
>> environments drop tests with duplicated name.
>>
>> When working on adding subtests support in Netdev's NIPA CI, I found out
>> some subtests had the same name. The name is used as a key, it should
>> then be unique, otherwise we don't track duplicated tests.
>>
>> The fixes have been split in different patches, because it fixes
>> different commits. But the idea is always the same: make sure subtest
>> names are unique.
>>
>> The last patch is not for -net, that's a new feature to make sure we
>> don't add duplicated names later.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Matthieu Baerts (NGI0) (5):
>>       selftests: mptcp: simult flows: fix some subtest names
>>       selftests: mptcp: userspace_pm: unique subtest names
>>       selftests: mptcp: diag: unique 'in use' subtest names
>>       selftests: mptcp: diag: unique 'cestab' subtest names
>>       selftests: mptcp: lib: catch duplicated subtest entries
> 
> Hi Matt,
> 
> I'd like to review this series this week.

Thank you!

> Please delegate them for
> me on Patchwork. I can't delegate them by myself.

Sorry, I'm still not able to do that, we need to wait for kernel.org IT
team to do the modification.

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

Re: [PATCH mptcp-net 1/5] selftests: mptcp: simult flows: fix some subtest names

[Matthieu Baerts]

Hi Geliang,

Thank you for the review!

On 13/02/2024 12:34, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, Feb 09, 2024 at 06:28:39PM +0100, Matthieu Baerts (NGI0) wrote:
>> The selftest was correctly recording all the results, but the 'reverse
>> direction' part was missing in the name when needed.
>>
>> It is important to have a unique (sub)test name in TAP, because some CI
>> environments drop tests with duplicated name.
>>
>> Fixes: 675d99338e7a ("selftests: mptcp: simult flows: format subtests results in TAP")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/simult_flows.sh | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
>> index 619be0e1acf5..f377ef01970b 100755
>> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
>> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
>> @@ -250,7 +250,8 @@ run_test()
>>  		[ $bail -eq 0 ] || exit $ret
>>  	fi
>>  
>> -	printf "%-60s" "$msg - reverse direction"
>> +	msg="${msg} - reverse direction"
> 
> How about using '+=' operator to append a string like:
> 
> 	msg+=" - reverse direction"
> 
> The same in patch 3 and patch 4:
> 
> 	msg+=" after flush"
> 
> WDYT?

I usually prefer not to do that, because that's Bash specific, and it
doesn't work with 'sh'.

Here, we explicitly use '/bin/bash', so nothing is blocking me, just an
habit when often switching between "bash" and "sh" :)
I can do the modification if you prefer.

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

Re: [PATCH mptcp-net 1/5] selftests: mptcp: simult flows: fix some subtest names

[Geliang Tang]

Hi Matt,

On Tue, Feb 13, 2024 at 01:06:40PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 13/02/2024 12:34, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Fri, Feb 09, 2024 at 06:28:39PM +0100, Matthieu Baerts (NGI0) wrote:
> >> The selftest was correctly recording all the results, but the 'reverse
> >> direction' part was missing in the name when needed.
> >>
> >> It is important to have a unique (sub)test name in TAP, because some CI
> >> environments drop tests with duplicated name.
> >>
> >> Fixes: 675d99338e7a ("selftests: mptcp: simult flows: format subtests results in TAP")
> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> >> ---
> >>  tools/testing/selftests/net/mptcp/simult_flows.sh | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> >> index 619be0e1acf5..f377ef01970b 100755
> >> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> >> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> >> @@ -250,7 +250,8 @@ run_test()
> >>  		[ $bail -eq 0 ] || exit $ret
> >>  	fi
> >>  
> >> -	printf "%-60s" "$msg - reverse direction"
> >> +	msg="${msg} - reverse direction"
> > 
> > How about using '+=' operator to append a string like:
> > 
> > 	msg+=" - reverse direction"
> > 
> > The same in patch 3 and patch 4:
> > 
> > 	msg+=" after flush"
> > 
> > WDYT?
> 
> I usually prefer not to do that, because that's Bash specific, and it
> doesn't work with 'sh'.
> 
> Here, we explicitly use '/bin/bash', so nothing is blocking me, just an
> habit when often switching between "bash" and "sh" :)
> I can do the modification if you prefer.

Both are fine to me. You can modify them or not when merging them. All
tests passed on my side.

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

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

Re: selftests: mptcp: lib: catch duplicated subtest entries: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): packetdrill_regressions 🔴:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7887527303

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0c1cb32e0aaa


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net 1/5] selftests: mptcp: simult flows: fix some subtest names

[Matthieu Baerts]

Hi Geliang,

Thank you for your reply!

On 13/02/2024 14:48, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, Feb 13, 2024 at 01:06:40PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review!
>>
>> On 13/02/2024 12:34, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Fri, Feb 09, 2024 at 06:28:39PM +0100, Matthieu Baerts (NGI0) wrote:
>>>> The selftest was correctly recording all the results, but the 'reverse
>>>> direction' part was missing in the name when needed.
>>>>
>>>> It is important to have a unique (sub)test name in TAP, because some CI
>>>> environments drop tests with duplicated name.
>>>>
>>>> Fixes: 675d99338e7a ("selftests: mptcp: simult flows: format subtests results in TAP")
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/mptcp/simult_flows.sh | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
>>>> index 619be0e1acf5..f377ef01970b 100755
>>>> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
>>>> @@ -250,7 +250,8 @@ run_test()
>>>>  		[ $bail -eq 0 ] || exit $ret
>>>>  	fi
>>>>  
>>>> -	printf "%-60s" "$msg - reverse direction"
>>>> +	msg="${msg} - reverse direction"
>>>
>>> How about using '+=' operator to append a string like:
>>>
>>> 	msg+=" - reverse direction"
>>>
>>> The same in patch 3 and patch 4:
>>>
>>> 	msg+=" after flush"
>>>
>>> WDYT?
>>
>> I usually prefer not to do that, because that's Bash specific, and it
>> doesn't work with 'sh'.
>>
>> Here, we explicitly use '/bin/bash', so nothing is blocking me, just an
>> habit when often switching between "bash" and "sh" :)
>> I can do the modification if you prefer.
> 
> Both are fine to me. You can modify them or not when merging them. All
> tests passed on my side.

I noticed we were using the '+=' operator with strings in different
places in our selftests, so good to continue! I did the modification you
suggested in patches 1, 3 and 4.

I just applied the series in our tree (patches 1-4 in 'fixes for -net'
and patch 5 in 'features for net-next'), with your RvB tag.

New patches for t/upstream-net and t/upstream:
- 44927e6860b0: selftests: mptcp: simult flows: fix some subtest names
- 87e468d41e57: selftests: mptcp: userspace_pm: unique subtest names
- 177281100c09: selftests: mptcp: diag: unique 'in use' subtest names
- 766166e63987: selftests: mptcp: diag: unique 'cestab' subtest names
- Results: 4a5e7bd9b1b6..0675fccde951 (export-net)
- Results: 5051f12eb83c..0d848ef42dd7 (export)

New patches for t/upstream only:
- c5f0aa0fe0a0: selftests: mptcp: lib: catch duplicated subtest entries
- Results: 0d848ef42dd7..f63a9c1dbbaa (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20240213T151258
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20240213T151856

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

Re: [PATCH mptcp-net 0/5] selftests: mptcp: unique subtest names

[Geliang Tang]

On Fri, Feb 09, 2024 at 06:28:38PM +0100, Matthieu Baerts (NGI0) wrote:
> It is important to have a unique (sub)test name in TAP, because some CI
> environments drop tests with duplicated name.
> 
> When working on adding subtests support in Netdev's NIPA CI, I found out
> some subtests had the same name. The name is used as a key, it should
> then be unique, otherwise we don't track duplicated tests.
> 
> The fixes have been split in different patches, because it fixes
> different commits. But the idea is always the same: make sure subtest
> names are unique.
> 
> The last patch is not for -net, that's a new feature to make sure we
> don't add duplicated names later.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (5):
>       selftests: mptcp: simult flows: fix some subtest names
>       selftests: mptcp: userspace_pm: unique subtest names
>       selftests: mptcp: diag: unique 'in use' subtest names
>       selftests: mptcp: diag: unique 'cestab' subtest names
>       selftests: mptcp: lib: catch duplicated subtest entries

LGTM!

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> 
>  tools/testing/selftests/net/mptcp/diag.sh         | 37 ++++++++++++++---------
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh    | 21 +++++++++++++
>  tools/testing/selftests/net/mptcp/simult_flows.sh |  3 +-
>  tools/testing/selftests/net/mptcp/userspace_pm.sh |  4 +--
>  4 files changed, 48 insertions(+), 17 deletions(-)
> ---
> base-commit: 0c8d1475f726a95cddb393e45f0b4d8ec0e15785
> change-id: 20240209-selftests-mptcp-uniq-title-2e2df0c17695
> 
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 

Re: selftests: mptcp: lib: catch duplicated subtest entries: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI (Cirrus) did some validations with a debug kernel and here is its report:

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6204403636502528
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6204403636502528/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4599610446970880
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4599610446970880/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0c1cb32e0aaa


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)