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

[Eric Blake]

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.  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).

I'll hold off queuing the patch while conversation continues.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org