The initial goal of this RFC is to get feedback on tests not specific
to the QEMU main binary, but specific to other components such as
qemu-img.

For this experiment, a small issue with the zero and negative number
of I/O operations given to the bench command was chosen.

Cleber Rosa (2):
  Acceptance Tests: add QemuImgTest base class
  qemu-img: consider a zero number of I/O requests an invalid count

 qemu-img.c                                  |   8 ++---
 tests/acceptance/avocado_qemu/__init__.py   |  20 ++++++++++++
 tests/acceptance/qemu_img_bench.py          |  34 ++++++++++++++++++++
 tests/acceptance/qemu_img_bench.py.data/img |   1 +
 tests/data/images/empty/raw                 | Bin 0 -> 1024 bytes
 5 files changed, 59 insertions(+), 4 deletions(-)
 create mode 100644 tests/acceptance/qemu_img_bench.py
 create mode 120000 tests/acceptance/qemu_img_bench.py.data/img
 create mode 100644 tests/data/images/empty/raw

-- 
2.19.1

Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben:
> The initial goal of this RFC is to get feedback on tests not specific
> to the QEMU main binary, but specific to other components such as
> qemu-img.
> 
> For this experiment, a small issue with the zero and negative number
> of I/O operations given to the bench command was chosen.

Any reason why this shouldn't be in qemu-iotests?

Kevin

On 11/12/18 5:49 AM, Kevin Wolf wrote:
> Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben:
>> The initial goal of this RFC is to get feedback on tests not specific
>> to the QEMU main binary, but specific to other components such as
>> qemu-img.
>>
>> For this experiment, a small issue with the zero and negative number
>> of I/O operations given to the bench command was chosen.
> 
> Any reason why this shouldn't be in qemu-iotests?
> 
> Kevin
> 

Hi Kevin,

This is indeed one of the comments I was expecting to receive.  AFAIK,
there's nothing that prevents such a *simple* test to be written as a
qemu-iotest.

Having said that, one of the things we're trying to achieve with
"tests/acceptance" is that a individual developer or maintainer, should
be able to run a subset of tests that he/she cares about.

Suppose that this developer is working on a "snapshot" related feature,
and wants to run tests that cover both "qemu-img snapshot" and then
tests interacting with a guest running on a snapshotted image.  By using
the tags mechanism, one could run:

 $ avocado run -t snapshot tests/acceptance

And run all tests related to snapshot.  This is one of the reasons for
maybe allowing the type of test proposed here to live under
"tests/acceptance".  Others include:

 * No numbering conflicts when naming tests
 * More descriptive tests names and metadata
 * No "context switch" for people also writing acceptance tests
 * The various utility APIs available in both the Test class and on
avocado.utils

BTW, since most tests Today exist outside of "tests/acceptance", that
may be also be solved in a great part by adding support in the (Avocado)
test runner about some metadata in tests such qemu-iotests.

Cheers,
- Cleber.

On Mon, Nov 12, 2018 at 09:59:56AM -0500, Cleber Rosa wrote:
> 
> On 11/12/18 5:49 AM, Kevin Wolf wrote:
> > Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben:
> >> The initial goal of this RFC is to get feedback on tests not specific
> >> to the QEMU main binary, but specific to other components such as
> >> qemu-img.
> >>
> >> For this experiment, a small issue with the zero and negative number
> >> of I/O operations given to the bench command was chosen.
> > 
> > Any reason why this shouldn't be in qemu-iotests?
> > 
> > Kevin
> > 
> 
> Hi Kevin,
> 
> This is indeed one of the comments I was expecting to receive.  AFAIK,
> there's nothing that prevents such a *simple* test to be written as a
> qemu-iotest.
> 
> Having said that, one of the things we're trying to achieve with
> "tests/acceptance" is that a individual developer or maintainer, should
> be able to run a subset of tests that he/she cares about.
> 
> Suppose that this developer is working on a "snapshot" related feature,
> and wants to run tests that cover both "qemu-img snapshot" and then
> tests interacting with a guest running on a snapshotted image.  By using
> the tags mechanism, one could run:
> 
>  $ avocado run -t snapshot tests/acceptance
> 
> And run all tests related to snapshot.  This is one of the reasons for
> maybe allowing the type of test proposed here to live under
> "tests/acceptance".  Others include:
> 
>  * No numbering conflicts when naming tests
>  * More descriptive tests names and metadata

I've long thought we should change the naming for tests/qemu-iotests to
have descriptive names instead just forever clashing 3-digit numbers
that generate needless merge conflits

We also already have a tagging concept for iotests in the "groups"
file. Thus far we've only used a few generic names, but we could
expand that at will, so no reason why we couldn't have a "snapshot"
group listed there.

Personally I'd get rid of the groups file though, and just use magic
comments at the top of each test. This would again reduce needless
merge conflicts.

If people are doing work on the block layer I think they'll already
be used to runnng qemu-iotests as their acceptance test check. There
are already many there which only focus on qemu-img/qemu-io, and not
the QEMU system emulators

>  * No "context switch" for people also writing acceptance tests
>  * The various utility APIs available in both the Test class and on
> avocado.utils
> 
> BTW, since most tests Today exist outside of "tests/acceptance", that
> may be also be solved in a great part by adding support in the (Avocado)
> test runner about some metadata in tests such qemu-iotests.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Am 12.11.2018 um 15:59 hat Cleber Rosa geschrieben:
> 
> On 11/12/18 5:49 AM, Kevin Wolf wrote:
> > Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben:
> >> The initial goal of this RFC is to get feedback on tests not specific
> >> to the QEMU main binary, but specific to other components such as
> >> qemu-img.
> >>
> >> For this experiment, a small issue with the zero and negative number
> >> of I/O operations given to the bench command was chosen.
> > 
> > Any reason why this shouldn't be in qemu-iotests?
> > 
> > Kevin
> > 
> 
> Hi Kevin,
> 
> This is indeed one of the comments I was expecting to receive.

I expected that you should expect this question. So it surprised me to
see that the cover letter didn't address it at all.

> AFAIK, there's nothing that prevents such a *simple* test to be
> written as a qemu-iotest.

Tests for qemu-img are pretty much by definition simple, just because
qemu-img isn't very complex (in particular, it doesn't run guests which
could introduce arbitrary complexity).

Can you give an example of what you would consider a not simple test for
qemu-img?

> Having said that, one of the things we're trying to achieve with
> "tests/acceptance" is that a individual developer or maintainer, should
> be able to run a subset of tests that he/she cares about.
> 
> Suppose that this developer is working on a "snapshot" related feature,
> and wants to run tests that cover both "qemu-img snapshot" and then
> tests interacting with a guest running on a snapshotted image.  By using
> the tags mechanism, one could run:
> 
>  $ avocado run -t snapshot tests/acceptance

You mean like './check -g snapshot'? (It would be more useful if we had
cared enough to actually assign that group to some of the newer tests,
but it exists...)

> And run all tests related to snapshot.  This is one of the reasons for
> maybe allowing the type of test proposed here to live under
> "tests/acceptance".  Others include:
> 
>  * No numbering conflicts when naming tests
>  * More descriptive tests names and metadata

Test numbering and metadata - sure, we can change that in qemu-iotests.
Should be a lot easier than adding a whole new second infrastructure for
block tests.

>  * No "context switch" for people also writing acceptance tests

There are no people writing "acceptance tests" for the block layer yet.
The context switch comes only with your patches, since you are
introducing a second competing framework for the same task, without even
giving a clear path of how to integrate or convert the existing tests so
we could get back to a unified world.

I really don't think fragmenting the test infrastructure is a good idea,
especially for rather superficial advantages.

>  * The various utility APIs available in both the Test class and on
> avocado.utils

Can you give examples?

Are those utility APIs actually worth losing the existing iotests.py
functions that provide stuff that is pretty specific to the QEMU and the
block layer?

> BTW, since most tests Today exist outside of "tests/acceptance", that
> may be also be solved in a great part by adding support in the (Avocado)
> test runner about some metadata in tests such qemu-iotests.

Sure, feel free to wrap qemu-iotests as much as you want. Trivial
wrappers don't sound like a big maintenance burden.

I just think that the block layer tests themselves should still keep
living in a single place. The obvious one is qemu-iotests. If you want
to change that, your cover letter shouldn't be quite as terse. The least
I would expect is an elaborate answer to:

1. Why is a change to something completely new useful and worth the
   effort? We don't generally rewrite QEMU or the kernel if some parts
   of it are ugly. We incrementally improve it instead. Exceptions need
   good justification because rewrites come with costs, especially if
   they can't offer feature parity.

2. How do we migrate the existing tests to the new infrastructure to
   avoid fragmentation?

Kevin

On 11/12/18 11:00 AM, Kevin Wolf wrote:
> Am 12.11.2018 um 15:59 hat Cleber Rosa geschrieben:
>>
>> On 11/12/18 5:49 AM, Kevin Wolf wrote:
>>> Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben:
>>>> The initial goal of this RFC is to get feedback on tests not specific
>>>> to the QEMU main binary, but specific to other components such as
>>>> qemu-img.
>>>>
>>>> For this experiment, a small issue with the zero and negative number
>>>> of I/O operations given to the bench command was chosen.
>>>
>>> Any reason why this shouldn't be in qemu-iotests?
>>>
>>> Kevin
>>>
>>
>> Hi Kevin,
>>
>> This is indeed one of the comments I was expecting to receive.
> 
> I expected that you should expect this question. So it surprised me to
> see that the cover letter didn't address it at all.
> 

I hope you don't blame me for trying to have the advantage of the
counter answer. :)

>> AFAIK, there's nothing that prevents such a *simple* test to be
>> written as a qemu-iotest.
> 
> Tests for qemu-img are pretty much by definition simple, just because
> qemu-img isn't very complex (in particular, it doesn't run guests which
> could introduce arbitrary complexity).
> 
> Can you give an example of what you would consider a not simple test for
> qemu-img?
> 

This is a hard question for me to answer since I haven't written that
many qemu-img tests.  Thinking of hypothetical situations, the Avocado
libraries contain utilities for logical volumes[1], so it'd naturally
be easier to write a test for qemu-img + LVs.

>> Having said that, one of the things we're trying to achieve with
>> "tests/acceptance" is that a individual developer or maintainer, should
>> be able to run a subset of tests that he/she cares about.
>>
>> Suppose that this developer is working on a "snapshot" related feature,
>> and wants to run tests that cover both "qemu-img snapshot" and then
>> tests interacting with a guest running on a snapshotted image.  By using
>> the tags mechanism, one could run:
>>
>>  $ avocado run -t snapshot tests/acceptance
> 
> You mean like './check -g snapshot'? (It would be more useful if we had
> cared enough to actually assign that group to some of the newer tests,
> but it exists...)
> 

Yes, but also something equivalent to './check -g snapshot,live -g
quick,-privileged', meaning tests that are tagged both with "snapshot
and live", in addition to tests that are quick and don't require super
user privilege.

Don't get me wrong, I wouldn't expect "check" to implement that logic.
Like I said before, one way to solve that is to add leave qemu-iotests
untouched, and add support on the Avocado runner to understand other
tests' metadata.

>> And run all tests related to snapshot.  This is one of the reasons for
>> maybe allowing the type of test proposed here to live under
>> "tests/acceptance".  Others include:
>>
>>  * No numbering conflicts when naming tests
>>  * More descriptive tests names and metadata
> 
> Test numbering and metadata - sure, we can change that in qemu-iotests.
> Should be a lot easier than adding a whole new second infrastructure for
> block tests.
> 

My impression is that the "infrastructure for block tests" is not that
different from the infrastructure needed by other tests, specially other
QEMU tests.  The point I'm trying to make here is that, adding a feature
such as metadata parsing/selection to tests looks much more like a *test
infrastructure* issue than a "block test" issue, right?

Having access to local or remote files (images?), preparing the
environment (creating those images?), accepting parameters from the user
(which image format to use?) are all examples of test infrastructure
problems applied to the block tests.

Logging and formatting the results are other examples of *test*
infrastructure problems that "check" has had to deal with itself, and is
quite understandably limited.

>>  * No "context switch" for people also writing acceptance tests
> 
> There are no people writing "acceptance tests" for the block layer yet.
> The context switch comes only with your patches, since you are
> introducing a second competing framework for the same task, without even
> giving a clear path of how to integrate or convert the existing tests so
> we could get back to a unified world.
> 

You're absolutely right, and it's quite obvious that there's no one
writing "acceptance tests" for the block layer yet.  There's a subtle
but important difference here though: this initiative is trying to allow
people to write tests generic enough, for various QEMU subsystems
(that's why maybe it's badly named as "acceptance").  It's really about
trying to avoid context switches that may occur when developers and
maintainers from those various subsystems (hopefully) start to write and
review tests.

So no, this is not an attempt to cause disruption, fragmentation and
separate worlds.  It's quite the contrary.  And please excuse me from
not writing a "how to migrate qemu-iotests" --  I don't even want to
think about that if the block layer maintainers do not see any value in
that.

If you believe that handing off some of the infrastructure problems that
qemu-iotests have to a common tool, and that it may be a good idea to
have qemu-iotests become more like "qemu tests that happen to exercise
the block layer", then we can push such an initiative forward.

> I really don't think fragmenting the test infrastructure is a good idea,
> especially for rather superficial advantages.
> 
>>  * The various utility APIs available in both the Test class and on
>> avocado.utils
> 
> Can you give examples?
> 

I believe ended up giving some examples before, but for ease reading,
let me duplicate them here:

 * avocado.Test: "Having access to local or remote files (images?),
preparing the environment (creating those images?), accepting parameters
from the user (which image format to use?) are all examples of test
infrastructure problems applied to the block tests."

 * avocado.utils: "Thinking of hypothetical situations, the Avocado
libraries contain utilities for logical volumes[1], so it'd naturally
be easier to write a test for qemu-img + LVs."

> Are those utility APIs actually worth losing the existing iotests.py
> functions that provide stuff that is pretty specific to the QEMU and the
> block layer?
> 

There's no reason to lose iotests.py.  Even the current acceptance tests
are based on the principle of reusing the code that a lot of the iotests
use (scripts/qemu.py and scripts/qmp/*).

What I'm aiming for is that QEMU developers can write *tests*, and have
a simple (and hopefully a common) way of running them.

>> BTW, since most tests Today exist outside of "tests/acceptance", that
>> may be also be solved in a great part by adding support in the (Avocado)
>> test runner about some metadata in tests such qemu-iotests.
> 
> Sure, feel free to wrap qemu-iotests as much as you want. Trivial
> wrappers don't sound like a big maintenance burden.
> 
> I just think that the block layer tests themselves should still keep
> living in a single place. The obvious one is qemu-iotests. If you want
> to change that, your cover letter shouldn't be quite as terse. The least
> I would expect is an elaborate answer to:
> 
> 1. Why is a change to something completely new useful and worth the
>    effort? We don't generally rewrite QEMU or the kernel if some parts
>    of it are ugly. We incrementally improve it instead. Exceptions need
>    good justification because rewrites come with costs, especially if
>    they can't offer feature parity.
> 

I'll tell you a "shocking secret" (please take that with a grain of
salt): I have no use case myself for any of the QEMU tests.  I don't
rely on them.  No QEMU test make a difference in my work.  That's why I
may miss some of the obvious points, but at the same time, maybe I can
look at things from a different perspective.

Because incrementally improving the *overall* testing of QEMU is indeed
one of my goals, this RFC was a "provocation" for change.  Had I written
a Python script with a very similar content, named it "233", we'd have
missed this discussion.

> 2. How do we migrate the existing tests to the new infrastructure to
>    avoid fragmentation?
> 

Again, I haven't even thought of proposing that.  This is so dependent
on so many other aspects, including this initial feeling and feedback
from the maintainers.

> Kevin
> 

Thanks a lot for the feedback so far,
- Cleber.

[1]
https://avocado-framework.readthedocs.io/en/65.0/api/utils/avocado.utils.html#module-avocado.utils.lv_utils

Drive-by comment...

Cleber Rosa <crosa@redhat.com> writes:

[...]
> My impression is that the "infrastructure for block tests" is not that
> different from the infrastructure needed by other tests, specially other
> QEMU tests.
[...]

Yes.  The actual reason for having a completely separate testing
infrastructure for block tests is that it predates testing
infrastructure for anything else.

Moving the tests to common infrastructure would be a sizable one-time
effort we can ill afford.  Maintaining multiple testing infrastructures
is an ongoing effort we can also ill afford.

Perhaps moving to common infrastructure is impractical.  I don't know.
What I do know is beware of temporal discounting.

[...]

On Tue, Nov 13, 2018 at 10:39:57AM +0100, Markus Armbruster wrote:
> Drive-by comment...
> 
> Cleber Rosa <crosa@redhat.com> writes:
> 
> [...]
> > My impression is that the "infrastructure for block tests" is not that
> > different from the infrastructure needed by other tests, specially other
> > QEMU tests.
> [...]
> 
> Yes.  The actual reason for having a completely separate testing
> infrastructure for block tests is that it predates testing
> infrastructure for anything else.
> 
> Moving the tests to common infrastructure would be a sizable one-time
> effort we can ill afford.  Maintaining multiple testing infrastructures
> is an ongoing effort we can also ill afford.

If we do want to move to a common infrastructure, then IMHO this patch
series should do the work to convert qemu-iotests to use it.

Currently this is simply introducing a 2nd way to write block tests.

We know from bitter experiance that when we introduce a new system in
QEMU without converting the old system, the old system will live forever
increasing our maint burden & creatnig confusion for developers about
which is the preferred approach.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 11/13/18 8:50 AM, Daniel P. Berrangé wrote:
> On Tue, Nov 13, 2018 at 10:39:57AM +0100, Markus Armbruster wrote:
>> Drive-by comment...
>>
>> Cleber Rosa <crosa@redhat.com> writes:
>>
>> [...]
>>> My impression is that the "infrastructure for block tests" is not that
>>> different from the infrastructure needed by other tests, specially other
>>> QEMU tests.
>> [...]
>>
>> Yes.  The actual reason for having a completely separate testing
>> infrastructure for block tests is that it predates testing
>> infrastructure for anything else.
>>
>> Moving the tests to common infrastructure would be a sizable one-time
>> effort we can ill afford.  Maintaining multiple testing infrastructures
>> is an ongoing effort we can also ill afford.
> 
> If we do want to move to a common infrastructure, then IMHO this patch
> series should do the work to convert qemu-iotests to use it.
> 
> Currently this is simply introducing a 2nd way to write block tests.
> 
> We know from bitter experiance that when we introduce a new system in
> QEMU without converting the old system, the old system will live forever
> increasing our maint burden & creatnig confusion for developers about
> which is the preferred approach.
> 
> 
> Regards,
> Daniel
> 

Right, I don't think there's enough momentum or interest in doing that
right now.  As myself and Kevin suggested, the best solution at this
point may be wrapping the qemu-iotests without touching them.

Regards,
- Cleber.

Am 12.11.2018 um 18:36 hat Cleber Rosa geschrieben:
> I hope you don't blame me for trying to have the advantage of the
> counter answer. :)

Thanks for being so honest, but do you actually need this advantage when
you have good technical arguments in favour of your proposal?

> >> And run all tests related to snapshot.  This is one of the reasons for
> >> maybe allowing the type of test proposed here to live under
> >> "tests/acceptance".  Others include:
> >>
> >>  * No numbering conflicts when naming tests
> >>  * More descriptive tests names and metadata
> > 
> > Test numbering and metadata - sure, we can change that in qemu-iotests.
> > Should be a lot easier than adding a whole new second infrastructure for
> > block tests.
> > 
> 
> My impression is that the "infrastructure for block tests" is not that
> different from the infrastructure needed by other tests, specially other
> QEMU tests.  The point I'm trying to make here is that, adding a feature
> such as metadata parsing/selection to tests looks much more like a *test
> infrastructure* issue than a "block test" issue, right?

It depends on what you include in "infrastructure". qemu-iotests
has some functionality that is probably common to all test frameworks.
But it also has some functionality that is very specific to QEMU and the
block layer.

As a rough guideline, anything that ./check does outside the actual test
case is probably mostly generic; anything that is included in the test
case files (such as most common.* files and iotests.py) are domain
specific.

So yes, there are parts of qemu-iotests that could be covered by another
tool, and probably better, but there are also parts worth of keeping.

> >>  * No "context switch" for people also writing acceptance tests
> > 
> > There are no people writing "acceptance tests" for the block layer yet.
> > The context switch comes only with your patches, since you are
> > introducing a second competing framework for the same task, without even
> > giving a clear path of how to integrate or convert the existing tests so
> > we could get back to a unified world.
> > 
> 
> You're absolutely right, and it's quite obvious that there's no one
> writing "acceptance tests" for the block layer yet.  There's a subtle
> but important difference here though: this initiative is trying to allow
> people to write tests generic enough, for various QEMU subsystems
> (that's why maybe it's badly named as "acceptance").  It's really about
> trying to avoid context switches that may occur when developers and
> maintainers from those various subsystems (hopefully) start to write and
> review tests.
> 
> So no, this is not an attempt to cause disruption, fragmentation and
> separate worlds.  It's quite the contrary.  And please excuse me from
> not writing a "how to migrate qemu-iotests" --  I don't even want to
> think about that if the block layer maintainers do not see any value in
> that.

[ Insert that "how standards proliferate" xkcd here ]

I see value in having a unified test infrastructure. However, I don't
think committing a new Hello World grade qemu-img test case using
different tooling than the rest of the qemu-img test cases is
contributing much to this goal.

We have so many unfinished conversions inside QEMU, and this looks like
the approach you would take to add another one.

> If you believe that handing off some of the infrastructure problems that
> qemu-iotests have to a common tool, and that it may be a good idea to
> have qemu-iotests become more like "qemu tests that happen to exercise
> the block layer", then we can push such an initiative forward.

Fine with me. But let's push it forward in a different way then.

My primary concern is not being able to write new tests in a new
environment. Consistency between block tests is more important to me
than consistency of new block tests with tests for other parts of QEMU.
So my primary concern is how to make the existing roughly 232 test cases
in qemu-iotests compatible with the way the other parts use.

And in fact, as those other parts don't exist yet (other than simple
examples), maybe it would actually be worth discussing whether they
shouldn't use the same structure and tooling as qemu-iotests (which can
obviously be improved at the same time, but my point is about being in
sync) instead of setting a new standard without considering the existing
test cases and then trying to move qemu-iotests to it.

> > Are those utility APIs actually worth losing the existing iotests.py
> > functions that provide stuff that is pretty specific to the QEMU and the
> > block layer?
> 
> There's no reason to lose iotests.py.  Even the current acceptance tests
> are based on the principle of reusing the code that a lot of the iotests
> use (scripts/qemu.py and scripts/qmp/*).
> 
> What I'm aiming for is that QEMU developers can write *tests*, and have
> a simple (and hopefully a common) way of running them.

We already do write tests. It's not like you're starting from zero, even
if your approach is as if you did.

Anyway, one specific concern about the "simple way" I have is that we're
adding a hard dependency on an external package (Avocado) that isn't
usually installed anyway on developer machines. Maintainers will of
course just install it. But will this reduce the amount of tests that
contributors run, and increase the amount of untested patches on the
mailing list?

Maybe we can keep a simple in-tree runner like ./check that doesn't have
any external dependencies and runs all of those tests that don't make
use of Avocado utility functions etc.? And you'd use Avocado when you
want to run all tests or use advanced test harness options.

> > 1. Why is a change to something completely new useful and worth the
> >    effort? We don't generally rewrite QEMU or the kernel if some parts
> >    of it are ugly. We incrementally improve it instead. Exceptions need
> >    good justification because rewrites come with costs, especially if
> >    they can't offer feature parity.
> > 
> 
> I'll tell you a "shocking secret" (please take that with a grain of
> salt): I have no use case myself for any of the QEMU tests.  I don't
> rely on them.  No QEMU test make a difference in my work.  That's why I
> may miss some of the obvious points, but at the same time, maybe I can
> look at things from a different perspective.
> 
> Because incrementally improving the *overall* testing of QEMU is indeed
> one of my goals, this RFC was a "provocation" for change.  Had I written
> a Python script with a very similar content, named it "233", we'd have
> missed this discussion.

I would still have had input for you, which I will now put here, because
I want to make sure that way we would write the test case will actually
be well supported (having to care about this would be avoided if we
started from "use the qemu-iotests test harness for everything" and then
incrementally improved it to make it suit better for other cases and to
make it usable from Avocado - in fact, the existing acceptance tests
could trivially fit in there without any necessary changes to the
harness).

So for a long time, the only Python test cases we had looked much like
your test case: Just run some code with lots of asserts in it, and at
the end print something like "42 test cases passed"; this or an
exception stack trace is the only output you get. Turns out that this is
really hard to debug when a test case starts to fail.

Our shell based test cases, on the other hand, have always printed the
output and then we diffed it against a reference output. When it fails,
you usually can see immediately what happens. The lesson we learned is
that most of our newer Python test cases are much more verbose and make
more use of diffing aginst a reference output instead of asserting. This
is what your qemu-img test should probably do as well.

And in fact, if it doesn't involve interacting with QMP in complex ways
that actually need some programming to deal with the return values of
the monitor, we wouldn't even write it in Python, but just in bash.

> > 2. How do we migrate the existing tests to the new infrastructure to
> >    avoid fragmentation?
> 
> Again, I haven't even thought of proposing that.  This is so dependent
> on so many other aspects, including this initial feeling and feedback
> from the maintainers.

But this is the most important point if we don't want to cause
fragmentation of the tests!

The answer to it decides whether doing any of this makes sense.

Kevin

On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote:
[...]
> Anyway, one specific concern about the "simple way" I have is that we're
> adding a hard dependency on an external package (Avocado) that isn't
> usually installed anyway on developer machines. Maintainers will of
> course just install it. But will this reduce the amount of tests that
> contributors run, and increase the amount of untested patches on the
> mailing list?
> 
> Maybe we can keep a simple in-tree runner like ./check that doesn't have
> any external dependencies and runs all of those tests that don't make
> use of Avocado utility functions etc.? And you'd use Avocado when you
> want to run all tests or use advanced test harness options.

What problems you are trying to address here, exactly?

If you don't have Avocado installed in your system, all you need
is Python 3, an internet connection, and the ability to type
"make check-acceptance" on your keyboard.

-- 
Eduardo

Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben:
> On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote:
> [...]
> > Anyway, one specific concern about the "simple way" I have is that we're
> > adding a hard dependency on an external package (Avocado) that isn't
> > usually installed anyway on developer machines. Maintainers will of
> > course just install it. But will this reduce the amount of tests that
> > contributors run, and increase the amount of untested patches on the
> > mailing list?
> > 
> > Maybe we can keep a simple in-tree runner like ./check that doesn't have
> > any external dependencies and runs all of those tests that don't make
> > use of Avocado utility functions etc.? And you'd use Avocado when you
> > want to run all tests or use advanced test harness options.
> 
> What problems you are trying to address here, exactly?
> 
> If you don't have Avocado installed in your system, all you need
> is Python 3, an internet connection, and the ability to type
> "make check-acceptance" on your keyboard.

Thanks, didn't know that one. Apparently you don't only need to have
Python 3 available on the system, but also explicitly use it for
./configure?

    $ LANG=C make check-acceptance
    /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3".  Stop.

While this doesn't make the tests available automatically for everyone,
we'll get there when we finally make Python 3 the default (hopefully
soon), which is already a lot better than what docs/devel/testing.rst
promises:

    These tests are written using the Avocado Testing Framework (which
    must be installed separately) [...]

Maybe time to update the docs to match the improved situation? :-)

Kevin

On Tue, Nov 13, 2018 at 02:51:16PM +0100, Kevin Wolf wrote:
> Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben:
> > On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote:
> > [...]
> > > Anyway, one specific concern about the "simple way" I have is that we're
> > > adding a hard dependency on an external package (Avocado) that isn't
> > > usually installed anyway on developer machines. Maintainers will of
> > > course just install it. But will this reduce the amount of tests that
> > > contributors run, and increase the amount of untested patches on the
> > > mailing list?
> > > 
> > > Maybe we can keep a simple in-tree runner like ./check that doesn't have
> > > any external dependencies and runs all of those tests that don't make
> > > use of Avocado utility functions etc.? And you'd use Avocado when you
> > > want to run all tests or use advanced test harness options.
> > 
> > What problems you are trying to address here, exactly?
> > 
> > If you don't have Avocado installed in your system, all you need
> > is Python 3, an internet connection, and the ability to type
> > "make check-acceptance" on your keyboard.
> 
> Thanks, didn't know that one. Apparently you don't only need to have
> Python 3 available on the system, but also explicitly use it for
> ./configure?
> 
>     $ LANG=C make check-acceptance
>     /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3".  Stop.

I suggested in another thread that we should simply use "python3"
instead of $(PYTHON) here, to solve that problem.  I think we can
use you reply as evidence that this is really the right thing to
do.  :)

> 
> While this doesn't make the tests available automatically for everyone,
> we'll get there when we finally make Python 3 the default (hopefully
> soon), which is already a lot better than what docs/devel/testing.rst
> promises:
> 
>     These tests are written using the Avocado Testing Framework (which
>     must be installed separately) [...]
> 
> Maybe time to update the docs to match the improved situation? :-)

Absolutely.  Thanks for the feedback!

-- 
Eduardo

On 11/13/18 8:51 AM, Kevin Wolf wrote:
> Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben:
>> On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote:
>> [...]
>>> Anyway, one specific concern about the "simple way" I have is that we're
>>> adding a hard dependency on an external package (Avocado) that isn't
>>> usually installed anyway on developer machines. Maintainers will of
>>> course just install it. But will this reduce the amount of tests that
>>> contributors run, and increase the amount of untested patches on the
>>> mailing list?
>>>
>>> Maybe we can keep a simple in-tree runner like ./check that doesn't have
>>> any external dependencies and runs all of those tests that don't make
>>> use of Avocado utility functions etc.? And you'd use Avocado when you
>>> want to run all tests or use advanced test harness options.
>>
>> What problems you are trying to address here, exactly?
>>
>> If you don't have Avocado installed in your system, all you need
>> is Python 3, an internet connection, and the ability to type
>> "make check-acceptance" on your keyboard.
> 
> Thanks, didn't know that one. Apparently you don't only need to have
> Python 3 available on the system, but also explicitly use it for
> ./configure?
> 
>     $ LANG=C make check-acceptance
>     /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3".  Stop.
> 
> While this doesn't make the tests available automatically for everyone,
> we'll get there when we finally make Python 3 the default (hopefully
> soon), which is already a lot better than what docs/devel/testing.rst
> promises:
> 
>     These tests are written using the Avocado Testing Framework (which
>     must be installed separately) [...]
> 
> Maybe time to update the docs to match the improved situation? :-)
> 
> Kevin
> 

The docs were updated when "make check-acceptance" was used.  That
sentence is still true, Avocado must be installed separately, it's just
that "check-venv", used by "check-acceptance" does just that.

It's documented as:

Running tests
-------------

You can run the acceptance tests simply by executing:

.. code::

  make check-acceptance

- Cleber.

On Tue, Nov 13, 2018 at 09:20:11AM -0500, Cleber Rosa wrote:
> 
> 
> On 11/13/18 8:51 AM, Kevin Wolf wrote:
> > Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben:
> >> On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote:
> >> [...]
> >>> Anyway, one specific concern about the "simple way" I have is that we're
> >>> adding a hard dependency on an external package (Avocado) that isn't
> >>> usually installed anyway on developer machines. Maintainers will of
> >>> course just install it. But will this reduce the amount of tests that
> >>> contributors run, and increase the amount of untested patches on the
> >>> mailing list?
> >>>
> >>> Maybe we can keep a simple in-tree runner like ./check that doesn't have
> >>> any external dependencies and runs all of those tests that don't make
> >>> use of Avocado utility functions etc.? And you'd use Avocado when you
> >>> want to run all tests or use advanced test harness options.
> >>
> >> What problems you are trying to address here, exactly?
> >>
> >> If you don't have Avocado installed in your system, all you need
> >> is Python 3, an internet connection, and the ability to type
> >> "make check-acceptance" on your keyboard.
> > 
> > Thanks, didn't know that one. Apparently you don't only need to have
> > Python 3 available on the system, but also explicitly use it for
> > ./configure?
> > 
> >     $ LANG=C make check-acceptance
> >     /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3".  Stop.
> > 
> > While this doesn't make the tests available automatically for everyone,
> > we'll get there when we finally make Python 3 the default (hopefully
> > soon), which is already a lot better than what docs/devel/testing.rst
> > promises:
> > 
> >     These tests are written using the Avocado Testing Framework (which
> >     must be installed separately) [...]
> > 
> > Maybe time to update the docs to match the improved situation? :-)
> > 
> > Kevin
> > 
> 
> The docs were updated when "make check-acceptance" was used.  That
> sentence is still true, Avocado must be installed separately, it's just
> that "check-venv", used by "check-acceptance" does just that.

With check-venv, we made "installing avocado" a small
implementation detail that people don't need to care about when
running the tests.

I believe the sentence "which must be installed separately" is
now confusing and misleading.  It can be easily interpreted as
"must be installed manually by the user", which is not case.

> [...]

-- 
Eduardo

On 11/13/18 9:32 AM, Eduardo Habkost wrote:
> On Tue, Nov 13, 2018 at 09:20:11AM -0500, Cleber Rosa wrote:
> 
> With check-venv, we made "installing avocado" a small
> implementation detail that people don't need to care about when
> running the tests.
> 
> I believe the sentence "which must be installed separately" is
> now confusing and misleading.  It can be easily interpreted as
> "must be installed manually by the user", which is not case.
> 

Yeah, it can be confusing.  Maybe something like:

These tests are written using the Avocado Testing Framework (which must
either be manually installed, or automatically as part of ``make
check-acceptance``)...

- Cleber.

On Tue, Nov 13, 2018 at 09:43:49AM -0500, Cleber Rosa wrote:
> 
> On 11/13/18 9:32 AM, Eduardo Habkost wrote:
> > On Tue, Nov 13, 2018 at 09:20:11AM -0500, Cleber Rosa wrote:
> > 
> > With check-venv, we made "installing avocado" a small
> > implementation detail that people don't need to care about when
> > running the tests.
> > 
> > I believe the sentence "which must be installed separately" is
> > now confusing and misleading.  It can be easily interpreted as
> > "must be installed manually by the user", which is not case.
> > 
> 
> Yeah, it can be confusing.  Maybe something like:
> 
> These tests are written using the Avocado Testing Framework (which must
> either be manually installed, or automatically as part of ``make
> check-acceptance``)...

If it doesn't require any action from the user, why bother
mentioning it at the beginning of the document?

Avocado installation can be a small footnote below the "running
tests" section, for people interested in implementation details.

-- 
Eduardo

On 11/13/18 7:18 AM, Kevin Wolf wrote:
> Am 12.11.2018 um 18:36 hat Cleber Rosa geschrieben:
>> I hope you don't blame me for trying to have the advantage of the
>> counter answer. :)
> 
> Thanks for being so honest, but do you actually need this advantage when
> you have good technical arguments in favour of your proposal?
> 

The smile should have given away that I was joking.  An advantage point
over others is not needed when those are hoping to work together, and
not trying to win an argument.

>>>> And run all tests related to snapshot.  This is one of the reasons for
>>>> maybe allowing the type of test proposed here to live under
>>>> "tests/acceptance".  Others include:
>>>>
>>>>  * No numbering conflicts when naming tests
>>>>  * More descriptive tests names and metadata
>>>
>>> Test numbering and metadata - sure, we can change that in qemu-iotests.
>>> Should be a lot easier than adding a whole new second infrastructure for
>>> block tests.
>>>
>>
>> My impression is that the "infrastructure for block tests" is not that
>> different from the infrastructure needed by other tests, specially other
>> QEMU tests.  The point I'm trying to make here is that, adding a feature
>> such as metadata parsing/selection to tests looks much more like a *test
>> infrastructure* issue than a "block test" issue, right?
> 
> It depends on what you include in "infrastructure". qemu-iotests
> has some functionality that is probably common to all test frameworks.
> But it also has some functionality that is very specific to QEMU and the
> block layer.
> 
> As a rough guideline, anything that ./check does outside the actual test
> case is probably mostly generic; anything that is included in the test
> case files (such as most common.* files and iotests.py) are domain
> specific.
> 
> So yes, there are parts of qemu-iotests that could be covered by another
> tool, and probably better, but there are also parts worth of keeping.
> 

Agreed.

>>>>  * No "context switch" for people also writing acceptance tests
>>>
>>> There are no people writing "acceptance tests" for the block layer yet.
>>> The context switch comes only with your patches, since you are
>>> introducing a second competing framework for the same task, without even
>>> giving a clear path of how to integrate or convert the existing tests so
>>> we could get back to a unified world.
>>>
>>
>> You're absolutely right, and it's quite obvious that there's no one
>> writing "acceptance tests" for the block layer yet.  There's a subtle
>> but important difference here though: this initiative is trying to allow
>> people to write tests generic enough, for various QEMU subsystems
>> (that's why maybe it's badly named as "acceptance").  It's really about
>> trying to avoid context switches that may occur when developers and
>> maintainers from those various subsystems (hopefully) start to write and
>> review tests.
>>
>> So no, this is not an attempt to cause disruption, fragmentation and
>> separate worlds.  It's quite the contrary.  And please excuse me from
>> not writing a "how to migrate qemu-iotests" --  I don't even want to
>> think about that if the block layer maintainers do not see any value in
>> that.
> 
> [ Insert that "how standards proliferate" xkcd here ]
> 
> I see value in having a unified test infrastructure. However, I don't
> think committing a new Hello World grade qemu-img test case using
> different tooling than the rest of the qemu-img test cases is
> contributing much to this goal.
> 

There are a few ways I take this response.  First one is that my attempt
to make this *RFC* an attempt to discuss and eventually collaborate, is
more a miss than a hit.  Second, it makes me wonder whether you'd prefer
to take a patch without any test, or a test using a "different tooling"
(I'm considering you do take patches without tests, if you don't, then
please disregard this point).

> We have so many unfinished conversions inside QEMU, and this looks like
> the approach you would take to add another one.
> 

Wrong, there's no conversion attempted here.

>> If you believe that handing off some of the infrastructure problems that
>> qemu-iotests have to a common tool, and that it may be a good idea to
>> have qemu-iotests become more like "qemu tests that happen to exercise
>> the block layer", then we can push such an initiative forward.
> 
> Fine with me. But let's push it forward in a different way then.
> 
> My primary concern is not being able to write new tests in a new
> environment. Consistency between block tests is more important to me
> than consistency of new block tests with tests for other parts of QEMU.
> So my primary concern is how to make the existing roughly 232 test cases
> in qemu-iotests compatible with the way the other parts use.
> 

OK.

> And in fact, as those other parts don't exist yet (other than simple
> examples), maybe it would actually be worth discussing whether they
> shouldn't use the same structure and tooling as qemu-iotests (which can
> obviously be improved at the same time, but my point is about being in

Feel free to start the discussion on why they should, and how.  I'm
really receptive to discussing any ideas.

> sync) instead of setting a new standard without considering the existing
> test cases and then trying to move qemu-iotests to it.
> 

OK, "new" having a bad connotation is quite common on your reply.  I
with I had succeeded at passing the idea that I valued qemu-iotests so
much that I was excited about hearing your thoughts about this
*possible* "new" thing.  "Without considering" is an unfair statement
when you get CC'd and when expected feedback is anticipated and valued.

>>> Are those utility APIs actually worth losing the existing iotests.py
>>> functions that provide stuff that is pretty specific to the QEMU and the
>>> block layer?
>>
>> There's no reason to lose iotests.py.  Even the current acceptance tests
>> are based on the principle of reusing the code that a lot of the iotests
>> use (scripts/qemu.py and scripts/qmp/*).
>>
>> What I'm aiming for is that QEMU developers can write *tests*, and have
>> a simple (and hopefully a common) way of running them.
> 
> We already do write tests. It's not like you're starting from zero, even
> if your approach is as if you did.
> 

I'm really disappointed that I have to explain this, because I'm either
failing miserably at communicating, or there's some other worse issues
going on here.

Anyway, when I used *tests* without a qualifier, I'm really talking
about lowering the specialization factors on the test infrastructure.
The giveaway is in adjectives such as "simple" and "common".  Please,
pretty please, assume positive intentions: I never intended or claimed
to be the pioneer of tests in QEMU.

> Anyway, one specific concern about the "simple way" I have is that we're
> adding a hard dependency on an external package (Avocado) that isn't
> usually installed anyway on developer machines. Maintainers will of
> course just install it. But will this reduce the amount of tests that
> contributors run, and increase the amount of untested patches on the
> mailing list?
> 

We've opted for a "opt-in" approach.  If you want to run tests that
depend on this external package, you go ahead and pay the data costs
associated with that download.  We did pack all those steps in a single
"make check-acceptance" command for everyone's convenience, though.

> Maybe we can keep a simple in-tree runner like ./check that doesn't have
> any external dependencies and runs all of those tests that don't make
> use of Avocado utility functions etc.? And you'd use Avocado when you
> want to run all tests or use advanced test harness options.
> 

Sure, this is the current state of affairs.  And yes, one way of
integrating the execution of test from different "silos" is by adding
support on the runner, as mentioned before.  I hope to work on this some
time soon.

>>> 1. Why is a change to something completely new useful and worth the
>>>    effort? We don't generally rewrite QEMU or the kernel if some parts
>>>    of it are ugly. We incrementally improve it instead. Exceptions need
>>>    good justification because rewrites come with costs, especially if
>>>    they can't offer feature parity.
>>>
>>
>> I'll tell you a "shocking secret" (please take that with a grain of
>> salt): I have no use case myself for any of the QEMU tests.  I don't
>> rely on them.  No QEMU test make a difference in my work.  That's why I
>> may miss some of the obvious points, but at the same time, maybe I can
>> look at things from a different perspective.
>>
>> Because incrementally improving the *overall* testing of QEMU is indeed
>> one of my goals, this RFC was a "provocation" for change.  Had I written
>> a Python script with a very similar content, named it "233", we'd have
>> missed this discussion.
> 
> I would still have had input for you, which I will now put here, because
> I want to make sure that way we would write the test case will actually
> be well supported (having to care about this would be avoided if we
> started from "use the qemu-iotests test harness for everything" and then
> incrementally improved it to make it suit better for other cases and to
> make it usable from Avocado - in fact, the existing acceptance tests
> could trivially fit in there without any necessary changes to the
> harness).
> 

Would you believe that most of the tests that a QE team runs (look at
Avocado-VT tests) would require no changes to the "check" harness or no
significant amount of supporting code?  It's easy to take into account
the existing few and simple acceptance tests, and disregard the
infrastructure needed, but we do have a road map that was part of the
original series cover letter.  We do have WiP versions of tests that
configure and boot various guests and interact with them.

It's tricky to get all of them at once at the quality level for
upstream, so what you see it's just the start, not the end goal.  With
that in mind, we consciously recognized that "./check" was not enough,
and it made no sense to put all that supporting code in QEMU itself.

> So for a long time, the only Python test cases we had looked much like
> your test case: Just run some code with lots of asserts in it, and at
> the end print something like "42 test cases passed"; this or an
> exception stack trace is the only output you get. Turns out that this is
> really hard to debug when a test case starts to fail.
> 
> Our shell based test cases, on the other hand, have always printed the
> output and then we diffed it against a reference output. When it fails,
> you usually can see immediately what happens. The lesson we learned is
> that most of our newer Python test cases are much more verbose and make
> more use of diffing aginst a reference output instead of asserting. This
> is what your qemu-img test should probably do as well.
> 
> And in fact, if it doesn't involve interacting with QMP in complex ways
> that actually need some programming to deal with the return values of
> the monitor, we wouldn't even write it in Python, but just in bash.
> 

Right, you'd review this new qemu-iotest, but the overall discussion
about an eventual larger collaboration would be missed.

>>> 2. How do we migrate the existing tests to the new infrastructure to
>>>    avoid fragmentation?
>>
>> Again, I haven't even thought of proposing that.  This is so dependent
>> on so many other aspects, including this initial feeling and feedback
>> from the maintainers.
> 
> But this is the most important point if we don't want to cause
> fragmentation of the tests!
> 
> The answer to it decides whether doing any of this makes sense.
> 
> Kevin
> 

Sorry but I can't commit at this point to writing such a proposal.
Maybe at a later time, priorities and common interests will better align
and we can revisit this topic.  I appreciate your time and feedback here
though.

Thanks,
- Cleber.

Hi Cleber,

I will shorten this email a lot while replying because I have the
impression that most of the discussion isn't actually as productive as
it could be. I'm not trying to evade on any point that I'm cutting out,
so if there is something specific in the part I'm removing that you
would like to get answer for, please let me know and I'll follow up.

Am 13.11.2018 um 15:15 hat Cleber Rosa geschrieben:
> >> So no, this is not an attempt to cause disruption, fragmentation and
> >> separate worlds.  It's quite the contrary.  And please excuse me from
> >> not writing a "how to migrate qemu-iotests" --  I don't even want to
> >> think about that if the block layer maintainers do not see any value in
> >> that.
> > 
> > [ Insert that "how standards proliferate" xkcd here ]
> > 
> > I see value in having a unified test infrastructure. However, I don't
> > think committing a new Hello World grade qemu-img test case using
> > different tooling than the rest of the qemu-img test cases is
> > contributing much to this goal.
> 
> There are a few ways I take this response.  First one is that my attempt
> to make this *RFC* an attempt to discuss and eventually collaborate, is
> more a miss than a hit.  Second, it makes me wonder whether you'd prefer
> to take a patch without any test, or a test using a "different tooling"
> (I'm considering you do take patches without tests, if you don't, then
> please disregard this point).

As for the actual patch series at hand, if you want it to get into 3.1,
the best way would be to split code change and test into two different
patches and make a traditional qemu-iotests case out of it. But I think
that wasn't the main point of the exercise, right?

For the broader topic of testing and the block layer, I believe the
following paragraph shows a fundamental misunderstanding. Maybe I'm
wrong, but that's my impression. This one:

> > We have so many unfinished conversions inside QEMU, and this looks
> > like the approach you would take to add another one.
> 
> Wrong, there's no conversion attempted here.

I get the impression that you think not attempting any conversion is
something that should make it easier for me to agree with the approach.

The opposite is true: Introducing a second way to do things and not
converting existing test cases to it so that the first way could go away
is the fragmentation that I really want to avoid. I don't mind too much
which way we use, but I do care a lot that it's a single way.


So when I said "let's push it forward in a different way", what I had
in mind was starting with the existing qemu-iotests:

Currently, if you run avocado in tests/qemu-iotests/, it doesn't do
anything. I imagine it shouldn't be very hard to make it run at least
the Python-based unit test style cases, which look very similar to what
we have in tests/acceptance/.

In the next step, we could do the Python-based tests that work with
diffing a reference output. And then shell scripts (which need a very
specific environment to run and might require the most changes).

And finally, we would make sure that every option in ./check has a
translation to the new style and start making use of the new features
that Avocado brings in.


There are questions to answer on the way that don't seem completely
clear to me, but that's probably just because I don't know Avocado very
well. For example, while tests like the VNC one run exactly once and
don't require a prepared environment, ./check from qemu-iotests takes
a parameter for the image format and protocol to use for the test case.
This mechanism would have to be translated to Avocado somehow.

I think if you (or someone else who know Avocado well) try to do this
change, you'll figure out solutions quickly. This is where we need your
help because the block layer people don't know how this stuff works (at
least I don't). Or if you don't have the time to actually do the
patches yourself, tell us how we should do it. I'm not sure I can find
the time for this, but I can try.


So if the most important part for you is not the qemu-img bench fix, but
the approach to testing in general, I think this would be a much more
useful way to attack it.

> > And in fact, as those other parts don't exist yet (other than simple
> > examples), maybe it would actually be worth discussing whether they
> > shouldn't use the same structure and tooling as qemu-iotests (which can
> > obviously be improved at the same time, but my point is about being in
> 
> Feel free to start the discussion on why they should, and how.  I'm
> really receptive to discussing any ideas.

Maybe it's actually too late for that, considering that you said that
many test cases are already in development that use the newly introduced
acceptance test way.

But it would have essentially been:

1. move the qemu-iotests infrastructure up to tests/
2. if you already want to add vnc tests, you can add them to that
3. do all of the conversion steps I outlined above to make sure that we
   don't lose features compared to the old infrastructure.
4. you get a generic infrastructure that looks much like what you're
   creating inside tests/acceptance, only that it's used for everything

We can still do it out of order, but we have two parts to integrate now
instead of just one that needs to be evolved.

(By the way, why is tests/acceptance/ a separate directory? If we want
it to be _the_ way to do tests, wouldn't it make more sense to have it
on the top level and then use subdirectories only to categorise test
cases?)

> > sync) instead of setting a new standard without considering the existing
> > test cases and then trying to move qemu-iotests to it.
> > 
> 
> OK, "new" having a bad connotation is quite common on your reply.

To me, "new" has a bad connotation as long as it implies that it
coexists with "old" instead of replacing it. This is the part that I
really want to fix, and then we can happily go ahead with "new" (and
only "new").

Experience says that the easiest way to get there is by simply evolving
"old" into "new", but there are other ways. Maybe the coexisting "new"
is already developed far enough that we will have to use other ways this
time. But then let's use those ways and not put up with "old" and "new"
coexisting.

> > Anyway, one specific concern about the "simple way" I have is that we're
> > adding a hard dependency on an external package (Avocado) that isn't
> > usually installed anyway on developer machines. Maintainers will of
> > course just install it. But will this reduce the amount of tests that
> > contributors run, and increase the amount of untested patches on the
> > mailing list?
> > 
> 
> We've opted for a "opt-in" approach.  If you want to run tests that
> depend on this external package, you go ahead and pay the data costs
> associated with that download.  We did pack all those steps in a single
> "make check-acceptance" command for everyone's convenience, though.
> 
> > Maybe we can keep a simple in-tree runner like ./check that doesn't have
> > any external dependencies and runs all of those tests that don't make
> > use of Avocado utility functions etc.? And you'd use Avocado when you
> > want to run all tests or use advanced test harness options.
> > 
> 
> Sure, this is the current state of affairs.  And yes, one way of
> integrating the execution of test from different "silos" is by adding
> support on the runner, as mentioned before.  I hope to work on this some
> time soon.

Actually, with Eduardo's hint that you don't need to manually install
Avocado any more, but we automatically download everything that is
needed, I consider this concern mostly addressed. We need to make sure
that ./configure without any options allows to run it (currently you
need to explicitly specify Python 3), but that's it.

Then a separate in-tree runner isn't really needed either.

> > So for a long time, the only Python test cases we had looked much like
> > your test case: Just run some code with lots of asserts in it, and at
> > the end print something like "42 test cases passed"; this or an
> > exception stack trace is the only output you get. Turns out that this is
> > really hard to debug when a test case starts to fail.
> > 
> > Our shell based test cases, on the other hand, have always printed the
> > output and then we diffed it against a reference output. When it fails,
> > you usually can see immediately what happens. The lesson we learned is
> > that most of our newer Python test cases are much more verbose and make
> > more use of diffing aginst a reference output instead of asserting. This
> > is what your qemu-img test should probably do as well.
> > 
> > And in fact, if it doesn't involve interacting with QMP in complex ways
> > that actually need some programming to deal with the return values of
> > the monitor, we wouldn't even write it in Python, but just in bash.
> 
> Right, you'd review this new qemu-iotest, but the overall discussion
> about an eventual larger collaboration would be missed.

This was not at all about the harness, but specific feedback how I would
like a simple qemu-img test case to work. The implied message, if any,
was that I'd like to see an example for how to implement a diff-based
test (ideally in bash) in the Avocado-based test framework.

At the same time, this would be the prototype for converting existing
qemu-iotests cases.

Kevin