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

[Matthieu Baerts <matttbe@kernel.org>]

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.