[RFC,net-next,mlxsw,03/14] selftests: forwarding: README: Document customization

Commit Message

Petr Machata March 25, 2024, 5:29 p.m. UTC

That any sort of customization is possible at all, let alone how it should
be done, is currently not at all clear. Document the whats and hows in
README.

Signed-off-by: Petr Machata <[e-mail]>
Reviewed-by: Benjamin Poirier <[e-mail]>
---
 tools/testing/selftests/net/forwarding/README | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Jakub Kicinski March 26, 2024, 12:34 a.m. UTC | #1

On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:
> +The forwarding selftests framework uses a number of variables that
> +influence its behavior and tools it invokes, and how it invokes them, in
> +various ways. A number of these variables can be overridden. The way these
> +overridable variables are specified is typically one of the following two
> +syntaxes:
> +
> +	: "${VARIABLE:=default_value}"
> +	VARIABLE=${VARIABLE:=default_value}
> +
> +Any of these variables can be overridden. Notably net/forwarding/lib.sh and
> +net/lib.sh contain a number of overridable variables.
> +
> +One way of overriding these variables is through the environment:
> +
> +	PAUSE_ON_FAIL=yes ./some_test.sh

I like this conversion a lot. Makes me want to propose that we make this
a standard feature of kselftest. If "env" file exists in the test
directory kselftest would load its contents before running every test.

That's more of a broader question to anyone reading on linux-kselftest@
if there's no interest more than happy to merge as is :)

> +The variable NETIFS is special. Since it is an array variable, there is no
> +way to pass it through the environment. Its value can instead be given as
> +consecutive arguments to the selftest:
> +
> +	./some_test.sh swp{1..8}

Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.?
We can have lib.sh convert that into an array with a ugly-but-short
loop, it's a bit tempting to get rid of the exception.

Petr Machata March 26, 2024, 10:31 a.m. UTC | #2

Jakub Kicinski <[e-mail]> writes:

> On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:
>> +The forwarding selftests framework uses a number of variables that
>> +influence its behavior and tools it invokes, and how it invokes them, in
>> +various ways. A number of these variables can be overridden. The way these
>> +overridable variables are specified is typically one of the following two
>> +syntaxes:
>> +
>> +	: "${VARIABLE:=default_value}"
>> +	VARIABLE=${VARIABLE:=default_value}
>> +
>> +Any of these variables can be overridden. Notably net/forwarding/lib.sh and
>> +net/lib.sh contain a number of overridable variables.
>> +
>> +One way of overriding these variables is through the environment:
>> +
>> +	PAUSE_ON_FAIL=yes ./some_test.sh
>
> I like this conversion a lot. Makes me want to propose that we make this

Convention you mean?
Nothing was converted, this has always worked.

> a standard feature of kselftest. If "env" file exists in the test
> directory kselftest would load its contents before running every test.
>
> That's more of a broader question to anyone reading on linux-kselftest@
> if there's no interest more than happy to merge as is :)
>
>> +The variable NETIFS is special. Since it is an array variable, there is no
>> +way to pass it through the environment. Its value can instead be given as
>> +consecutive arguments to the selftest:
>> +
>> +	./some_test.sh swp{1..8}
>
> Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.?
> We can have lib.sh convert that into an array with a ugly-but-short
> loop, it's a bit tempting to get rid of the exception.

The exception is a bit annoying, yeah. But it works today, should stay,
and therefore should be documented, so the paragraph won't go away. I
use it all the time, too. I basically don't use the config file, I just
use the env overrides and the argv interface names. It's very handy.

The alternative is also very verbose:

	NETIF_1=swp1 NETIF_2=swp2 NETIF_3=swp3 [...] ./some_test.sh.

Maybe we could do this though?

	NETIFS="swp1 swp2 swp3 swp4 swp5 swp6 swp7 swp8" ./some_test.sh

And like this it won't make you want to pull your hair from all the
repetition:

	NETIFS=$(echo swp{1..8}) ./some_test.sh

But NETIFS is going to be a special case one way or another. That you
need to specify it through several variables, or a variable with a
special value, means you need to explain it as a special case in the
documentation. At which point you have two exceptions, and an
interaction between them, to describe.

Jakub Kicinski March 26, 2024, 2:13 p.m. UTC | #3

On Tue, 26 Mar 2024 11:31:31 +0100 Petr Machata wrote:
> Jakub Kicinski <[e-mail]> writes:
> 
> > On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:  
> >> +The forwarding selftests framework uses a number of variables that
> >> +influence its behavior and tools it invokes, and how it invokes them, in
> >> +various ways. A number of these variables can be overridden. The way these
> >> +overridable variables are specified is typically one of the following two
> >> +syntaxes:
> >> +
> >> +	: "${VARIABLE:=default_value}"
> >> +	VARIABLE=${VARIABLE:=default_value}
> >> +
> >> +Any of these variables can be overridden. Notably net/forwarding/lib.sh and
> >> +net/lib.sh contain a number of overridable variables.
> >> +
> >> +One way of overriding these variables is through the environment:
> >> +
> >> +	PAUSE_ON_FAIL=yes ./some_test.sh  
> >
> > I like this conversion a lot. Makes me want to propose that we make this  
> 
> Convention you mean?

Yes, sorry

> Nothing was converted, this has always worked.

Right, for forwarding and perhaps net.

> > a standard feature of kselftest. If "env" file exists in the test
> > directory kselftest would load its contents before running every test.
> >
> > That's more of a broader question to anyone reading on linux-kselftest@
> > if there's no interest more than happy to merge as is :)
> >  
> >> +The variable NETIFS is special. Since it is an array variable, there is no
> >> +way to pass it through the environment. Its value can instead be given as
> >> +consecutive arguments to the selftest:
> >> +
> >> +	./some_test.sh swp{1..8}  
> >
> > Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.?
> > We can have lib.sh convert that into an array with a ugly-but-short
> > loop, it's a bit tempting to get rid of the exception.  
> 
> The exception is a bit annoying, yeah. But it works today, should stay,
> and therefore should be documented, so the paragraph won't go away. I
> use it all the time, too. I basically don't use the config file, I just
> use the env overrides and the argv interface names. It's very handy.
> 
> The alternative is also very verbose:
> 
> 	NETIF_1=swp1 NETIF_2=swp2 NETIF_3=swp3 [...] ./some_test.sh.
> 
> Maybe we could do this though?
> 
> 	NETIFS="swp1 swp2 swp3 swp4 swp5 swp6 swp7 swp8" ./some_test.sh
> 
> And like this it won't make you want to pull your hair from all the
> repetition:
> 
> 	NETIFS=$(echo swp{1..8}) ./some_test.sh
> 
> But NETIFS is going to be a special case one way or another. That you
> need to specify it through several variables, or a variable with a
> special value, means you need to explain it as a special case in the
> documentation. At which point you have two exceptions, and an
> interaction between them, to describe.

I think there's some value in passing all inputs in the same way (thru
env rather than argv). I guess it's subjective, you're coding it up, 
so you can pick.

Petr Machata March 26, 2024, 5:32 p.m. UTC | #4

Jakub Kicinski <[e-mail]> writes:

> On Tue, 26 Mar 2024 11:31:31 +0100 Petr Machata wrote:
>> Jakub Kicinski <[e-mail]> writes:
>>
>> > a standard feature of kselftest. If "env" file exists in the test
>> > directory kselftest would load its contents before running every test.
>> >
>> > That's more of a broader question to anyone reading on linux-kselftest@
>> > if there's no interest more than happy to merge as is :)
>> >
>> > On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:
>> >
>> >> +The variable NETIFS is special. Since it is an array variable, there is no
>> >> +way to pass it through the environment. Its value can instead be given as
>> >> +consecutive arguments to the selftest:
>> >> +
>> >> +	./some_test.sh swp{1..8}  
>> >
>> > Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.?
>> > We can have lib.sh convert that into an array with a ugly-but-short
>> > loop, it's a bit tempting to get rid of the exception.  
>> 
>> Maybe we could do this though?
>> 
>> 	NETIFS="swp1 swp2 swp3 swp4 swp5 swp6 swp7 swp8" ./some_test.sh
>> 
>> But NETIFS is going to be a special case one way or another. That you
>> need to specify it through several variables, or a variable with a
>> special value, means you need to explain it as a special case in the
>> documentation. At which point you have two exceptions, and an
>> interaction between them, to describe.
>
> I think there's some value in passing all inputs in the same way (thru
> env rather than argv). I guess it's subjective, you're coding it up, 
> so you can pick.

I kinda like the NETIFS="a b c" approach. If somebody wants to code that
up, I'll be happy to review :) I might get around to it at some point.

Patch

diff --git a/tools/testing/selftests/net/forwarding/README b/tools/testing/selftests/net/forwarding/README
index b8a2af8fcfb7..7fdb6a9ca543 100644
--- a/tools/testing/selftests/net/forwarding/README
+++ b/tools/testing/selftests/net/forwarding/README
@@ -56,3 +56,36 @@  o Checks shall be added to lib.sh for any external dependencies.
 o Code shall be checked using ShellCheck [1] prior to submission.
 
 1. https://www.shellcheck.net/
+
+Customization
+=============
+
+The forwarding selftests framework uses a number of variables that
+influence its behavior and tools it invokes, and how it invokes them, in
+various ways. A number of these variables can be overridden. The way these
+overridable variables are specified is typically one of the following two
+syntaxes:
+
+	: "${VARIABLE:=default_value}"
+	VARIABLE=${VARIABLE:=default_value}
+
+Any of these variables can be overridden. Notably net/forwarding/lib.sh and
+net/lib.sh contain a number of overridable variables.
+
+One way of overriding these variables is through the environment:
+
+	PAUSE_ON_FAIL=yes ./some_test.sh
+
+The variable NETIFS is special. Since it is an array variable, there is no
+way to pass it through the environment. Its value can instead be given as
+consecutive arguments to the selftest:
+
+	./some_test.sh swp{1..8}
+
+A way to customize variables in a persistent fashion is to create a file
+named forwarding.config in this directory. lib.sh sources the file if
+present, so it can contain any shell code. Typically it will contain
+assignments of variables whose value should be overridden.
+
+forwarding.config.sample is available in the directory as an example of
+how forwarding.config might look.