selftests: mptcp: unique subtest names

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

Posted by Matthieu Baerts (NGI0) 8 months, 1 week ago

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

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

Posted by Geliang Tang 8 months, 1 week ago

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 1/5] selftests: mptcp: simult flows: fix some subtest names

Posted by Matthieu Baerts 8 months, 1 week ago

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

Posted by Geliang Tang 8 months, 1 week ago

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: [PATCH mptcp-net 1/5] selftests: mptcp: simult flows: fix some subtest names

Posted by Matthieu Baerts 8 months, 1 week ago

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.