Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Kevin Wolf <kwolf@redhat.com>]

Am 13.09.2023 um 18:31 hat Eric Blake geschrieben:
> On Wed, Sep 13, 2023 at 04:47:54PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
> > > Each particular testcase could skipped intentionally and accidentally.
> > > For example the test is not designed for a particular image format or
> > > is not run due to the missed library.
> > > 
> > > The latter case is unwanted in reality. Though the discussion has
> > > revealed that failing the test in such a case would be bad. Thus the
> > > patch tries to do different thing. It adds additional status for
> > > the test case - 'skipped' and bound intentinal cases to that state.
> > 
> > I'm not convinced this distinction makes sense and I fear it is
> > leading us down the same undesirable route as avocado with too
> > many distinct states leading to confusion:
> > 
> >   https://lore.kernel.org/qemu-devel/Yy1gB1KB3YSIUcoC@redhat.com/
> > 
> > If I looked at the output I can't tell you the difference between
> > "not run" and "skipped" - they both sound the same to me.
> > 
> > IMHO there's alot to be said for the simplicity of sticking with
> > nothing more than PASS/FAIL/SKIP as status names.  The descriptive
> > text associated with each SKIP would give more context as to the
> > reason in each case if needed.
> 
> I guess it boils down to whether there is an actionable response in
> that message.  If a test is skipped because it is the wrong format
> (for example, ./check -raw skipping a test that only works with
> qcow2), there's nothing for me to do.  If a test is skipped because my
> setup didn't permit running the test, but where I could enhance my
> environment (install more software, pick a different file system,
> ...), then having the skip message call that out is useful if I want
> to take action to get more test coverage.

I'm not sure if there is a clear line to define whether there is an
actionable response or not, because that completely depends on what the
user considers an acceptable response.

You have the relatively clear cases like the test being for another
format, though you could argue that the actionable response is running
./check -qcow2 if raw doesn't work - in fact, why don't we allow ./check
-raw -qcow2 to run both and count it as skipped only if it wasn't run at
all?

The relatively clear case on the other end of the spectrum is if a tool
is missing on the host that you could possibly install. Though maybe
your distro doesn't even package it, so is that still a reasonable
action to take?

What about cases where a block driver isn't compiled in or not
whitelisted? (We've seen this often enough with quorum.) That could be
an accident, but more likely it was a conscious decision and while just
enabling it might fix the test case, enabling additional features in
your product just to make tests happy might not be considered
acceptable. So should this be "not run" or "skipped"? [1]

You could find other unclear cases like tests depending on a different
target architecture, or testing different code paths on different target
architectures.

> Even if the message is present, we have so many tests intentionally
> skipped that it is hard to see the few tests where a skip could be
> turned into a pass by installing a prerequisite.
> 
> > > +++ b/tests/qemu-iotests/testrunner.py
> > > @@ -200,6 +200,8 @@ def test_print_one_line(self, test: str,
> > >                  col = '\033[1m\033[31m'
> > >              elif status == 'not run':
> > >                  col = '\033[33m'
> > > +            elif status == 'skipped':
> > > +                col = '\033[34m'
> > >              else:
> > >                  col = ''
> 
> It looks like for now, the only difference in the two designations is
> the output color, and even then, only if you are running the tests in
> an environment where color matters (CI systems may not be showing
> colors as intended).

Well, and the different status text, obviously.

CI will probably use the tap interface, which is not modified at all by
the patch, so no result would be recorded for "skipped" tests (while
"not run" tests will still return "ok ... # SKIP").

Kevin

[1] If you do answer this rhetorical question, please note that I
    already forgot which is which, so maybe also specify if you mean the
    bad skip or the harmless skip.