A few patches from the v1 have already been applied.

Compared to the v1, this series comes with new patches and updated ones.

Patch 1/9 was suggested by Paolo: bigger than the original one but following the
standard way of using getopt. This is also clearer and avoid regexes.

Patch 2/9: while working on patch 1/9, I was annoyed by having the tests groups
defined multiple times. And I think it already happened to have missed the
update of one of them :)

Patch 3/9 is "safer" but also needed with the new options to execute specific
tests.

Patch 4/9 is the updated version of the previous patch 3/9 from v1: mainly
rebased. I only added one line in the "usage()" function and use lower cases for
only_tests variable.

Patch 5/9 is new and similar to the previous patch: another way to execute a
specific test, useful when using 'git blame'. Also help to have patch 6/9.

Patch 6/9 is a handy feature when trying to find which tests had issues when
executed on a CI or locally.

Patch 7/9 was present in the v1. Non local variables have no longer been renamed
+ a there were a few missing 'local' keywords.

Patch 8/9 and 9/9 are the same as in v1 but rebased. We are still ShellCheck
compliant.

Matthieu Baerts (9):
  Squash to "selftests: mptcp: join: allow running -cCi"
  selftests: mptcp: join: define tests groups once
  selftests: mptcp: join: reset failing links
  selftests: mptcp: join: option to execute specific tests
  selftests: mptcp: join: alt. to exec specific tests
  selftests: mptcp: join: list failure at the end
  selftests: mptcp: join: clarify local/global vars
  selftests: mptcp: join: avoid backquotes
  selftests: mptcp: join: make it shellcheck compliant

 .../testing/selftests/net/mptcp/mptcp_join.sh | 2248 +++++++++--------
 1 file changed, 1202 insertions(+), 1046 deletions(-)

-- 
2.34.1

On Thu, 17 Feb 2022, Matthieu Baerts wrote:

> A few patches from the v1 have already been applied.
>
> Compared to the v1, this series comes with new patches and updated ones.
>
> Patch 1/9 was suggested by Paolo: bigger than the original one but following the
> standard way of using getopt. This is also clearer and avoid regexes.
>
> Patch 2/9: while working on patch 1/9, I was annoyed by having the tests groups
> defined multiple times. And I think it already happened to have missed the
> update of one of them :)
>
> Patch 3/9 is "safer" but also needed with the new options to execute specific
> tests.
>

Patches 1-3 look good to apply now:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> Patch 4/9 is the updated version of the previous patch 3/9 from v1: mainly
> rebased. I only added one line in the "usage()" function and use lower cases for
> only_tests variable.
>
> Patch 5/9 is new and similar to the previous patch: another way to execute a
> specific test, useful when using 'git blame'. Also help to have patch 6/9.
>

These are running fine for me, it's just the issue of 2000+ lines of churn 
and the potential (or even likely) net/net-next merge conflicts if we need 
to modify mptcp_join.sh after the 5.18 merge window. Hopefully all the 
work we're doing now to stabilize the tests means we will not have as many 
-net mptcp_join.sh changes in the next cycle, but we would still probably 
run in to some headaches. If we do apply these it would need to be right 
before net-next closes for 5.18.

Since you've put this much work into this patch it implies that you think 
the merge headaches would be worth it for the 5.18 development cycle :) 
For the way I use the test script the ability to test by number is not 
that critical - is anyone else (Paolo?) thinking you'll use this feature?


> Patch 6/9 is a handy feature when trying to find which tests had issues when
> executed on a CI or locally.
>

I like this! Highly dependent on previous patches though.

> Patch 7/9 was present in the v1. Non local variables have no longer been renamed
> + a there were a few missing 'local' keywords.
>

This seems a lot more manageable than v1, thanks.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> Patch 8/9 and 9/9 are the same as in v1 but rebased. We are still ShellCheck
> compliant.
>

These look good too:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> Matthieu Baerts (9):
>  Squash to "selftests: mptcp: join: allow running -cCi"
>  selftests: mptcp: join: define tests groups once
>  selftests: mptcp: join: reset failing links
>  selftests: mptcp: join: option to execute specific tests
>  selftests: mptcp: join: alt. to exec specific tests
>  selftests: mptcp: join: list failure at the end
>  selftests: mptcp: join: clarify local/global vars
>  selftests: mptcp: join: avoid backquotes
>  selftests: mptcp: join: make it shellcheck compliant
>
> .../testing/selftests/net/mptcp/mptcp_join.sh | 2248 +++++++++--------
> 1 file changed, 1202 insertions(+), 1046 deletions(-)
>
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

On Thu, 2022-02-17 at 16:33 -0800, Mat Martineau wrote:
> > Patch 4/9 is the updated version of the previous patch 3/9 from v1: mainly
> > rebased. I only added one line in the "usage()" function and use lower cases for
> > only_tests variable.
> > 
> > Patch 5/9 is new and similar to the previous patch: another way to execute a
> > specific test, useful when using 'git blame'. Also help to have patch 6/9.
> > 
> 
> These are running fine for me, it's just the issue of 2000+ lines of churn 
> and the potential (or even likely) net/net-next merge conflicts if we need 
> to modify mptcp_join.sh after the 5.18 merge window. Hopefully all the 
> work we're doing now to stabilize the tests means we will not have as many 
> -net mptcp_join.sh changes in the next cycle, but we would still probably 
> run in to some headaches. If we do apply these it would need to be right 
> before net-next closes for 5.18.
> 
> Since you've put this much work into this patch it implies that you think 
> the merge headaches would be worth it for the 5.18 development cycle :) 
> For the way I use the test script the ability to test by number is not 
> that critical - is anyone else (Paolo?) thinking you'll use this feature?

That feature would have saved a significant amount of my time in the
past weeks. I think the diffstat is a bit too much, and I still would
prefer the other approach of filtering out the test in each helper used
by the test themself.

Additionally, I think it would be nice replace the 'bare' ip netns exec
...' we still have in some the userspace tests with the appropriate
helpers - possibly worth a separate, independent patch.

/P
> 
> 
> > Patch 6/9 is a handy feature when trying to find which tests had issues when
> > executed on a CI or locally.
> > 
> 
> I like this! Highly dependent on previous patches though.
> 
> > Patch 7/9 was present in the v1. Non local variables have no longer been renamed
> > + a there were a few missing 'local' keywords.
> > 
> 
> This seems a lot more manageable than v1, thanks.
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> > Patch 8/9 and 9/9 are the same as in v1 but rebased. We are still ShellCheck
> > compliant.
> > 
> 
> These look good too:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> > Matthieu Baerts (9):
> >  Squash to "selftests: mptcp: join: allow running -cCi"
> >  selftests: mptcp: join: define tests groups once
> >  selftests: mptcp: join: reset failing links
> >  selftests: mptcp: join: option to execute specific tests
> >  selftests: mptcp: join: alt. to exec specific tests
> >  selftests: mptcp: join: list failure at the end
> >  selftests: mptcp: join: clarify local/global vars
> >  selftests: mptcp: join: avoid backquotes
> >  selftests: mptcp: join: make it shellcheck compliant
> > 
> > .../testing/selftests/net/mptcp/mptcp_join.sh | 2248 +++++++++--------
> > 1 file changed, 1202 insertions(+), 1046 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 
> > 
> > 
> 
> --
> Mat Martineau
> Intel
> 

On Fri, 2022-02-18 at 12:04 +0100, Paolo Abeni wrote:
> On Thu, 2022-02-17 at 16:33 -0800, Mat Martineau wrote:
> > > Patch 4/9 is the updated version of the previous patch 3/9 from v1: mainly
> > > rebased. I only added one line in the "usage()" function and use lower cases for
> > > only_tests variable.
> > > 
> > > Patch 5/9 is new and similar to the previous patch: another way to execute a
> > > specific test, useful when using 'git blame'. Also help to have patch 6/9.
> > > 
> > 
> > These are running fine for me, it's just the issue of 2000+ lines of churn 
> > and the potential (or even likely) net/net-next merge conflicts if we need 
> > to modify mptcp_join.sh after the 5.18 merge window. Hopefully all the 
> > work we're doing now to stabilize the tests means we will not have as many 
> > -net mptcp_join.sh changes in the next cycle, but we would still probably 
> > run in to some headaches. If we do apply these it would need to be right 
> > before net-next closes for 5.18.
> > 
> > Since you've put this much work into this patch it implies that you think 
> > the merge headaches would be worth it for the 5.18 development cycle :) 
> > For the way I use the test script the ability to test by number is not 
> > that critical - is anyone else (Paolo?) thinking you'll use this feature?
> 
> That feature would have saved a significant amount of my time in the
> past weeks. I think the diffstat is a bit too much, and I still would
> prefer the other approach of filtering out the test in each helper used
> by the test themself.

Uhmmm... my main argument vs the current version of patch 4 is defeated
by patches 5 && 6, which look useful. So I'm ok with patch 4 as is...

> Additionally, I think it would be nice replace the 'bare' ip netns exec
> ...' we still have in some the userspace tests with the appropriate
> helpers - possibly worth a separate, independent patch.

... still think it would be nice (almost necessary) cover the above ->
migrate all direct ip netns exec... usage to the appropriate helper.

/P

On Fri, 18 Feb 2022, Paolo Abeni wrote:

> On Fri, 2022-02-18 at 12:04 +0100, Paolo Abeni wrote:
>> On Thu, 2022-02-17 at 16:33 -0800, Mat Martineau wrote:
>>>> Patch 4/9 is the updated version of the previous patch 3/9 from v1: mainly
>>>> rebased. I only added one line in the "usage()" function and use lower cases for
>>>> only_tests variable.
>>>>
>>>> Patch 5/9 is new and similar to the previous patch: another way to execute a
>>>> specific test, useful when using 'git blame'. Also help to have patch 6/9.
>>>>
>>>
>>> These are running fine for me, it's just the issue of 2000+ lines of churn
>>> and the potential (or even likely) net/net-next merge conflicts if we need
>>> to modify mptcp_join.sh after the 5.18 merge window. Hopefully all the
>>> work we're doing now to stabilize the tests means we will not have as many
>>> -net mptcp_join.sh changes in the next cycle, but we would still probably
>>> run in to some headaches. If we do apply these it would need to be right
>>> before net-next closes for 5.18.
>>>
>>> Since you've put this much work into this patch it implies that you think
>>> the merge headaches would be worth it for the 5.18 development cycle :)
>>> For the way I use the test script the ability to test by number is not
>>> that critical - is anyone else (Paolo?) thinking you'll use this feature?
>>
>> That feature would have saved a significant amount of my time in the
>> past weeks. I think the diffstat is a bit too much, and I still would
>> prefer the other approach of filtering out the test in each helper used
>> by the test themself.
>
> Uhmmm... my main argument vs the current version of patch 4 is defeated
> by patches 5 && 6, which look useful. So I'm ok with patch 4 as is...
>

Ok, thanks for the insight! I do like the features, just wanted to make 
sure there's enough interest to justify the conflict resolution work we're 
signing up for. And it does seem like the payoff is worth it. At least 
it's a selftest script and not kernel code that could be disrupted by 
incorrect git conflict resolution here :)

>> Additionally, I think it would be nice replace the 'bare' ip netns exec
>> ...' we still have in some the userspace tests with the appropriate
>> helpers - possibly worth a separate, independent patch.
>
> ... still think it would be nice (almost necessary) cover the above ->
> migrate all direct ip netns exec... usage to the appropriate helper.
>

Sure, that would be good too.

Matthieu, I think it would be good to delay adding 4/5/6 to the export 
branch until mptcp_join.sh has stabilized in -net and been merged back to 
net-next - what do you think? Once these changes go in to the export 
branch we have a lot less flexibility in the order that patches go to 
net-next.

--
Mat Martineau
Intel

Hi Mat,

On 18/02/2022 19:59, Mat Martineau wrote:
> On Fri, 18 Feb 2022, Paolo Abeni wrote:
> 
>> On Fri, 2022-02-18 at 12:04 +0100, Paolo Abeni wrote:
>>> On Thu, 2022-02-17 at 16:33 -0800, Mat Martineau wrote:
>>>>> Patch 4/9 is the updated version of the previous patch 3/9 from v1:
>>>>> mainly
>>>>> rebased. I only added one line in the "usage()" function and use
>>>>> lower cases for
>>>>> only_tests variable.
>>>>>
>>>>> Patch 5/9 is new and similar to the previous patch: another way to
>>>>> execute a
>>>>> specific test, useful when using 'git blame'. Also help to have
>>>>> patch 6/9.
>>>>>
>>>>
>>>> These are running fine for me, it's just the issue of 2000+ lines of
>>>> churn
>>>> and the potential (or even likely) net/net-next merge conflicts if
>>>> we need
>>>> to modify mptcp_join.sh after the 5.18 merge window. Hopefully all the
>>>> work we're doing now to stabilize the tests means we will not have
>>>> as many
>>>> -net mptcp_join.sh changes in the next cycle, but we would still
>>>> probably
>>>> run in to some headaches. If we do apply these it would need to be
>>>> right
>>>> before net-next closes for 5.18.
>>>>
>>>> Since you've put this much work into this patch it implies that you
>>>> think
>>>> the merge headaches would be worth it for the 5.18 development cycle :)
>>>> For the way I use the test script the ability to test by number is not
>>>> that critical - is anyone else (Paolo?) thinking you'll use this
>>>> feature?
>>>
>>> That feature would have saved a significant amount of my time in the
>>> past weeks. I think the diffstat is a bit too much, and I still would
>>> prefer the other approach of filtering out the test in each helper used
>>> by the test themself.
>>
>> Uhmmm... my main argument vs the current version of patch 4 is defeated
>> by patches 5 && 6, which look useful. So I'm ok with patch 4 as is...
>>
> 
> Ok, thanks for the insight! I do like the features, just wanted to make
> sure there's enough interest to justify the conflict resolution work
> we're signing up for. And it does seem like the payoff is worth it. At
> least it's a selftest script and not kernel code that could be disrupted
> by incorrect git conflict resolution here :)

I think it is the good time to do that when you look at the diff with
the -net branch: a lot of other modifications in the same area have been
queued up in net-next :)

>>> Additionally, I think it would be nice replace the 'bare' ip netns exec
>>> ...' we still have in some the userspace tests with the appropriate
>>> helpers - possibly worth a separate, independent patch.
>>
>> ... still think it would be nice (almost necessary) cover the above ->
>> migrate all direct ip netns exec... usage to the appropriate helper.
>>
> 
> Sure, that would be good too.
> 
> Matthieu, I think it would be good to delay adding 4/5/6 to the export
> branch until mptcp_join.sh has stabilized in -net and been merged back
> to net-next - what do you think? Once these changes go in to the export
> branch we have a lot less flexibility in the order that patches go to
> net-next.

Sure I can. Do you mean waiting for the other patches that are in review
to fix issues in the tests?

I was thinking about that and I don't know if we need to wait: we
already have the commit 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp
wrappers") in net-next and there is no patches for -net that are
currently planned if I'm not mistaken.

If we still want to send them to net-next later, I can also apply these
patches after "git markup: features net-next": so just after new patches
that will come in the next days/weeks.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Fri, 18 Feb 2022, Matthieu Baerts wrote:

> Hi Mat,
>
> On 18/02/2022 19:59, Mat Martineau wrote:
>> On Fri, 18 Feb 2022, Paolo Abeni wrote:
>>
>>> On Fri, 2022-02-18 at 12:04 +0100, Paolo Abeni wrote:
>>>> On Thu, 2022-02-17 at 16:33 -0800, Mat Martineau wrote:
>>>>>> Patch 4/9 is the updated version of the previous patch 3/9 from v1:
>>>>>> mainly
>>>>>> rebased. I only added one line in the "usage()" function and use
>>>>>> lower cases for
>>>>>> only_tests variable.
>>>>>>
>>>>>> Patch 5/9 is new and similar to the previous patch: another way to
>>>>>> execute a
>>>>>> specific test, useful when using 'git blame'. Also help to have
>>>>>> patch 6/9.
>>>>>>
>>>>>
>>>>> These are running fine for me, it's just the issue of 2000+ lines of
>>>>> churn
>>>>> and the potential (or even likely) net/net-next merge conflicts if
>>>>> we need
>>>>> to modify mptcp_join.sh after the 5.18 merge window. Hopefully all the
>>>>> work we're doing now to stabilize the tests means we will not have
>>>>> as many
>>>>> -net mptcp_join.sh changes in the next cycle, but we would still
>>>>> probably
>>>>> run in to some headaches. If we do apply these it would need to be
>>>>> right
>>>>> before net-next closes for 5.18.
>>>>>
>>>>> Since you've put this much work into this patch it implies that you
>>>>> think
>>>>> the merge headaches would be worth it for the 5.18 development cycle :)
>>>>> For the way I use the test script the ability to test by number is not
>>>>> that critical - is anyone else (Paolo?) thinking you'll use this
>>>>> feature?
>>>>
>>>> That feature would have saved a significant amount of my time in the
>>>> past weeks. I think the diffstat is a bit too much, and I still would
>>>> prefer the other approach of filtering out the test in each helper used
>>>> by the test themself.
>>>
>>> Uhmmm... my main argument vs the current version of patch 4 is defeated
>>> by patches 5 && 6, which look useful. So I'm ok with patch 4 as is...
>>>
>>
>> Ok, thanks for the insight! I do like the features, just wanted to make
>> sure there's enough interest to justify the conflict resolution work
>> we're signing up for. And it does seem like the payoff is worth it. At
>> least it's a selftest script and not kernel code that could be disrupted
>> by incorrect git conflict resolution here :)
>
> I think it is the good time to do that when you look at the diff with
> the -net branch: a lot of other modifications in the same area have been
> queued up in net-next :)
>
>>>> Additionally, I think it would be nice replace the 'bare' ip netns exec
>>>> ...' we still have in some the userspace tests with the appropriate
>>>> helpers - possibly worth a separate, independent patch.
>>>
>>> ... still think it would be nice (almost necessary) cover the above ->
>>> migrate all direct ip netns exec... usage to the appropriate helper.
>>>
>>
>> Sure, that would be good too.
>>
>> Matthieu, I think it would be good to delay adding 4/5/6 to the export
>> branch until mptcp_join.sh has stabilized in -net and been merged back
>> to net-next - what do you think? Once these changes go in to the export
>> branch we have a lot less flexibility in the order that patches go to
>> net-next.
>
> Sure I can. Do you mean waiting for the other patches that are in review
> to fix issues in the tests?
>

Yes, that's part of it.

I can't send these for net-next until after these patches show up in 
net-next (via merge from -net):

https://lore.kernel.org/netdev/20220218213544.70285-1-mathew.j.martineau@linux.intel.com/T/

Any groups of patches touching mptcp_join.sh after these major-churn 
patches in the export branch would be blocked from net-next upstreaming 
until net/master and net-next/master are synced again.

Also, any mptcp_join.sh development work on the export branch would have 
similar dependencies. But I probably shouldn't worry about that too much.

> I was thinking about that and I don't know if we need to wait: we
> already have the commit 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp
> wrappers") in net-next and there is no patches for -net that are
> currently planned if I'm not mistaken.
>

-net patches are rarely planned :)

> If we still want to send them to net-next later, I can also apply these
> patches after "git markup: features net-next": so just after new patches
> that will come in the next days/weeks.
>

That should work. Thanks!

Also, to be clear for patches 4-6:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel