Re: [PATCH net-next v2 2/2] selftests: net: ksft: support marking tests as disruptive

[Petr Machata <petrm@nvidia.com>]

Stanislav Fomichev <sdf@fomichev.me> writes:

> On 07/31, Petr Machata wrote:
>> 
>> Stanislav Fomichev <sdf@fomichev.me> writes:
>> 
>> > Add new @ksft_disruptive decorator to mark the tests that might
>> > be disruptive to the system. Depending on how well the previous
>> > test works in the CI we might want to disable disruptive tests
>> > by default and only let the developers run them manually.
>> >
>> > KSFT framework runs disruptive tests by default. DISRUPTIVE=False
>> > environment (or config file) can be used to disable these tests.
>> > ksft_setup should be called by the test cases that want to use
>> > new decorator (ksft_setup is only called via NetDrvEnv/NetDrvEpEnv for now).
>> 
>> Is that something that tests would want to genuinely do, manage this
>> stuff by hand? I don't really mind having the helper globally
>> accessible, but default I'd keep it inside env.py and expect others to
>> inherit appropriately.
>
> Hard to say how well it's gonna work tbh. But at least from
> what I've seen, large code bases (outside of kernel) usually
> have some way to attach metadata to the testcase to indicate
> various things. For example, this is how the timeout
> can be controlled:
>
> https://bazel.build/reference/test-encyclopedia#role-test-runner
>
> So I'd imagine we can eventually have @kstf_short/@ksft_long to
> control that using similar techniques.
>
> Regarding keeping it inside env.py: can you expand more on what
> you mean by having the default in env.py?

I'm looking into it now and I missed how this is layered. ksft.py is the
comparatively general piece of code, and env.py is something
specifically for driver testing. It makes sense for ksft_setup() to be
where it is, because not-driver tests might want to be marked disruptive
as well. It also makes sense that env.py invokes the general helper.

All is good.

>> > @@ -127,6 +129,36 @@ KSFT_RESULT_ALL = True
>> >              KSFT_RESULT = False
>> >  
>> >  
>> > +def ksft_disruptive(func):
>> > +    """
>> > +    Decorator that marks the test as disruptive (e.g. the test
>> > +    that can down the interface). Disruptive tests can be skipped
>> > +    by passing DISRUPTIVE=False environment variable.
>> > +    """
>> > +
>> > +    @functools.wraps(func)
>> > +    def wrapper(*args, **kwargs):
>> > +        if not KSFT_DISRUPTIVE:
>> > +            raise KsftSkipEx(f"marked as disruptive")
>> 
>> Since this is a skip, it will fail the overall run. But that happened
>> because the user themselves set DISRUPTIVE=0 to avoid, um, disruption to
>> the system. I think it should either be xfail, or something else
>> dedicated that conveys the idea that we didn't run the test, but that's
>> fine.
>> 
>> Using xfail for this somehow doesn't seem correct, nothing failed. Maybe
>> we need KsftOmitEx, which would basically be an xfail with a more
>> appropriate name?
>
> Are you sure skip will fail the overall run? At least looking at
> tools/testing/selftests/net/lib/py/ksft.py, both skip and xfail are
> considered KSFT_RESULT=True. Or am I looking at the wrong place?

You seem to be right about the exit code. This was discussed some time
ago, that SKIP is considered a sort of a failure. As the person running
the test you would want to go in and fix whatever configuration issue is
preventing the test from running. I'm not sure how it works in practice,
whether people look for skips in the test log explicitly or rely on exit
codes.

Maybe Jakub can chime in, since he's the one that cajoled me into
handling this whole SKIP / XFAIL business properly in bash selftests.