selftests: mptcp: fixes for issues seen on v5.15.x

[PATCH mptcp-next 1/3] selftests: mptcp: pm nl: also list skipped tests

Posted by Matthieu Baerts (NGI0) 2 years ago

If the feature is not supported by older kernels, and instead of just
ignoring some tests, we should mark them as skipped, so we can still
track them.

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

diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 8f4ff123a7eb..79e83a2c95de 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -194,6 +194,12 @@ subflow 10.0.1.1" "          (nofullmesh)"
 	ip netns exec $ns1 ./pm_nl_ctl set id 1 flags backup,fullmesh
 	check "ip netns exec $ns1 ./pm_nl_ctl dump" "id 1 flags \
 subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
+else
+	for st in fullmesh nofullmesh backup,fullmesh; do
+		st="          (${st})"
+		printf "%-50s%s\n" "${st}" "[SKIP]"
+		mptcp_lib_result_skip "${st}"
+	done
 fi
 
 mptcp_lib_result_print_all_tap

-- 
2.43.0

Re: [PATCH mptcp-next 1/3] selftests: mptcp: pm nl: also list skipped tests

Posted by Geliang Tang 1 year, 12 months ago

Hi Matt,

Matthieu Baerts (NGI0) <matttbe@kernel.org> 于2024年2月8日周四 02:18写道：
>
> If the feature is not supported by older kernels, and instead of just
> ignoring some tests, we should mark them as skipped, so we can still
> track them.
>
> Fixes: d85555ac11f9 ("selftests: mptcp: pm_netlink: format subtests results in TAP")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/pm_netlink.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index 8f4ff123a7eb..79e83a2c95de 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> @@ -194,6 +194,12 @@ subflow 10.0.1.1" "          (nofullmesh)"
>         ip netns exec $ns1 ./pm_nl_ctl set id 1 flags backup,fullmesh
>         check "ip netns exec $ns1 ./pm_nl_ctl dump" "id 1 flags \
>  subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
> +else
> +       for st in fullmesh nofullmesh backup,fullmesh; do
> +               st="          (${st})"
> +               printf "%-50s%s\n" "${st}" "[SKIP]"

I prefer to use "[ SKIP ]" here. Otherwise, this series looks good!

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

Thanks,
-Geliang

> +               mptcp_lib_result_skip "${st}"
> +       done
>  fi
>
>  mptcp_lib_result_print_all_tap
>
> --
> 2.43.0
>
>

Re: [PATCH mptcp-next 1/3] selftests: mptcp: pm nl: also list skipped tests

Posted by Matthieu Baerts 1 year, 12 months ago

Hi Geliang,

Thank you for the review!

On 13/02/2024 14:35, Geliang Tang wrote:
> Hi Matt,
> 
> Matthieu Baerts (NGI0) <matttbe@kernel.org> 于2024年2月8日周四 02:18写道：
>>
>> If the feature is not supported by older kernels, and instead of just
>> ignoring some tests, we should mark them as skipped, so we can still
>> track them.
>>
>> Fixes: d85555ac11f9 ("selftests: mptcp: pm_netlink: format subtests results in TAP")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/pm_netlink.sh | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
>> index 8f4ff123a7eb..79e83a2c95de 100755
>> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
>> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
>> @@ -194,6 +194,12 @@ subflow 10.0.1.1" "          (nofullmesh)"
>>         ip netns exec $ns1 ./pm_nl_ctl set id 1 flags backup,fullmesh
>>         check "ip netns exec $ns1 ./pm_nl_ctl dump" "id 1 flags \
>>  subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
>> +else
>> +       for st in fullmesh nofullmesh backup,fullmesh; do
>> +               st="          (${st})"
>> +               printf "%-50s%s\n" "${st}" "[SKIP]"
> 
> I prefer to use "[ SKIP ]" here.

I would prefer too, but in this file, we are currently using:

  [ OK ]
  [FAIL]

I think it is then better to continue using 4 letters between the [], no?

  [ OK ]
  [FAIL]
  [SKIP]

If we want to change the format, and even add colours, we can do that in
net-next, not as part of a fix for -net.

> Otherwise, this series looks good!
>
> Reviewed-by: Geliang Tang <geliang@kernel.org>

If you don't mind, when a whole series is OK for you, can you share your
"Reviewed-By" tag on the cover-letter please? By doing that, your tag
will be "propagated" to each patch. While here, 'b4' will see it as only
valid for patch 1/3, but not the rest of the series.

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

Re: [PATCH mptcp-next 1/3] selftests: mptcp: pm nl: also list skipped tests

Posted by Matthieu Baerts 1 year, 12 months ago

Hi Geliang,

On 13/02/2024 15:59, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 13/02/2024 14:35, Geliang Tang wrote:
>> Hi Matt,
>>
>> Matthieu Baerts (NGI0) <matttbe@kernel.org> 于2024年2月8日周四 02:18写道：
>>>
>>> If the feature is not supported by older kernels, and instead of just
>>> ignoring some tests, we should mark them as skipped, so we can still
>>> track them.
>>>
>>> Fixes: d85555ac11f9 ("selftests: mptcp: pm_netlink: format subtests results in TAP")
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>> ---
>>>  tools/testing/selftests/net/mptcp/pm_netlink.sh | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
>>> index 8f4ff123a7eb..79e83a2c95de 100755
>>> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
>>> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
>>> @@ -194,6 +194,12 @@ subflow 10.0.1.1" "          (nofullmesh)"
>>>         ip netns exec $ns1 ./pm_nl_ctl set id 1 flags backup,fullmesh
>>>         check "ip netns exec $ns1 ./pm_nl_ctl dump" "id 1 flags \
>>>  subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
>>> +else
>>> +       for st in fullmesh nofullmesh backup,fullmesh; do
>>> +               st="          (${st})"
>>> +               printf "%-50s%s\n" "${st}" "[SKIP]"
>>
>> I prefer to use "[ SKIP ]" here.
> 
> I would prefer too, but in this file, we are currently using:
> 
>   [ OK ]
>   [FAIL]
> 
> I think it is then better to continue using 4 letters between the [], no?
> 
>   [ OK ]
>   [FAIL]
>   [SKIP]
> 
> If we want to change the format, and even add colours, we can do that in
> net-next, not as part of a fix for -net.

I hope that's OK: I just applied this series, with your RvB tag, in our
tree for -net. As always, I can always apply squash-to patches if
needed, if that was not OK for you (or anyone else).

New patches for t/upstream-net and t/upstream:
- 0d8688d3e27b: selftests: mptcp: pm nl: also list skipped tests
- 559d098b600a: selftests: mptcp: pm nl: avoid error msg on older kernels
- dc7dc31c08fa: selftests: mptcp: diag: fix bash warnings on older kernels
- Results: 65bddb3d48dc..4a5e7bd9b1b6 (export-net)
- Results: e1e18543311d..5051f12eb83c (export)

Tests are now in progress:

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

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