Re: [PATCH mptcp-next 3/9] selftests: mptcp: join: option to execute specific tests

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Mat, Paolo,

On 10/02/2022 11:04, Paolo Abeni wrote:
> On Wed, 2022-02-09 at 16:36 -0800, Mat Martineau wrote:
>> On Wed, 9 Feb 2022, Matthieu Baerts wrote:
>>
>>> Often, it is needed to run one specific test.
>>>
>>> There are options to run subgroups of tests but when only one fails, no
>>> need to run all the subgroup. So far, the solution was to edit the
>>> script to comment the tests that are not needed but that's not ideal.
>>>
>>> Now, it is possible to run one specific test by giving the ID of the
>>> tests that are going to be validated, e.g.
>>>
>>>  ./mptcp_join.sh 36 37
>>>
>>> This is cleaner and saves time.
>>>
>>> Technically, the reset* functions now return 0 if the test can be
>>> executed. This naturally creates sections per test in the code which is
>>> also helpful to understand what a test is exactly doing.
>>>
>>
>> My sense is that this is primarily for development purposes since the test 
>> numbers may shift as tests are added or modified. Is it worth the churn 
>> when we have the subgroup functionality and could comment out chunks of 
>> the test script that are not useful for running specific tests in a loop?

On my side, it would be helpful. On my side, I can edit the code but
that's not nice, especially if you want to automate tests with and
without a patch: you can have conflicts with the new comments, etc.

On the CI, I cannot easily modify the code so I end up testing more than
expected, that's slow and sometimes hits other issues.

I guess on Paolo's side too as he complained about this missing feature :)

>> I'm not set against this change, but hoping to discuss it some more to 
>> both understand the benefits (I assume for running CI in a loop?) and to 
>> see if anyone has bright ideas for avoiding the whitespace churn.
>>
>> ("Include a bash fork in our selftest code that implements 'goto'" seems 
>> like overkill :) )
> 
> Perhapas something alike the following ?!? Not a complete solution:
> - completely not tested
> - we will first need to add some wrapper for iptables or even beffer
> for ip netns exec, to be more generic
> - possibly more 'skip_test && return' needed, but not many, I guess.
> 
> I hope should be enough demonstrate the idea...

If we really want to avoid modifying existing lines in selftests, I can
also not add new tabs after the new "if reset" and drop patches 7-9. But
that's a shame we cannot refactor the code in the selftests :-/

I don't think it is a good idea to add the skip_test everywhere: we will
certainly forgot to add one somewhere until someone test the feature.

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