Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Stefan Hajnoczi]

On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> This adds some common functionality to control QEMU for qemu-iotests.
> 
> Additionally, test 085 is updated to use this new functionality.
> 
> Some minor fixups along the way, to clear up spaced pathname issues, 
> for common.rc, test 019, and test 086.
> 
> 
> Jeff Cody (3):
>   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

Once a test launches QEMU, it soon needs to parse QMP commands or wait
for QMP events.  That doesn't lend itself to the traditional
qemu-iotests shell model.  That is why iotests.py exists.

Shell script is a poor language for test cases that go beyond
pre-defined commands whose output is saved for diffing.  The string
manipulation is clumsy, JSON is not supported, tricks with fifos can
easily deadlock or break when a process terminates unexpectedly, etc.

If we go further in the direction of this patch series, we'll duplicate
existing iotests.py code and have complex shell tests that are hard to
extend.  I think it's time to draw the line and convert any test cases
that need to complexity to Python.

Why not use iotests.py?

Do you want help converting test cases that are on the edge to Python?

CCing Max because I think he was in favor of shell in the past and might
have some points here.

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Kevin Wolf]

Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > This adds some common functionality to control QEMU for qemu-iotests.
> > 
> > Additionally, test 085 is updated to use this new functionality.
> > 
> > Some minor fixups along the way, to clear up spaced pathname issues, 
> > for common.rc, test 019, and test 086.
> > 
> > 
> > Jeff Cody (3):
> >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> 
> Once a test launches QEMU, it soon needs to parse QMP commands or wait
> for QMP events.  That doesn't lend itself to the traditional
> qemu-iotests shell model.  That is why iotests.py exists.
> 
> Shell script is a poor language for test cases that go beyond
> pre-defined commands whose output is saved for diffing.  The string
> manipulation is clumsy, JSON is not supported, tricks with fifos can
> easily deadlock or break when a process terminates unexpectedly, etc.
> 
> If we go further in the direction of this patch series, we'll duplicate
> existing iotests.py code and have complex shell tests that are hard to
> extend.  I think it's time to draw the line and convert any test cases
> that need to complexity to Python.
> 
> Why not use iotests.py?

Because it's hard to use. The "compare against reference output" thing
is the first thing that you lose with iotests.py, and it's the most
useful feature in qemu-iotests.

When a Python test case fails, you get into real debugging. When a shell
script test case fails, you usually see immediately from the reference
output diff what's wrong.

I accept iotest.py for anything that needs to evaluate QMP return
values, reluctantly, because we have nothing better. But that's it, I
don't actually _like_ using it.

Kevin

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Jeff Cody]

On Mon, May 05, 2014 at 05:32:09PM +0200, Kevin Wolf wrote:
> Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> > On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > > This adds some common functionality to control QEMU for qemu-iotests.
> > > 
> > > Additionally, test 085 is updated to use this new functionality.
> > > 
> > > Some minor fixups along the way, to clear up spaced pathname issues, 
> > > for common.rc, test 019, and test 086.
> > > 
> > > 
> > > Jeff Cody (3):
> > >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> > 
> > Once a test launches QEMU, it soon needs to parse QMP commands or wait
> > for QMP events.  That doesn't lend itself to the traditional
> > qemu-iotests shell model.  That is why iotests.py exists.
> > 
> > Shell script is a poor language for test cases that go beyond
> > pre-defined commands whose output is saved for diffing.  The string
> > manipulation is clumsy, JSON is not supported, tricks with fifos can
> > easily deadlock or break when a process terminates unexpectedly, etc.
> > 
> > If we go further in the direction of this patch series, we'll duplicate
> > existing iotests.py code and have complex shell tests that are hard to
> > extend.  I think it's time to draw the line and convert any test cases
> > that need to complexity to Python.
> > 
> > Why not use iotests.py?
> 
> Because it's hard to use. The "compare against reference output" thing
> is the first thing that you lose with iotests.py, and it's the most
> useful feature in qemu-iotests.
> 
> When a Python test case fails, you get into real debugging. When a shell
> script test case fails, you usually see immediately from the reference
> output diff what's wrong.
> 
> I accept iotest.py for anything that needs to evaluate QMP return
> values, reluctantly, because we have nothing better. But that's it, I
> don't actually _like_ using it.
>

I agree with Kevin.  

Also, for a test framework, having some language duplication is not
necessarily a *bad* thing.  One, functionality may be tested in
slightly different ways, due to architectural differences.  Second, it
gives developers increased flexibility in writing test cases in a
language they may be more comfortable in - and that hopefully leads to
an increased number of tests being written.

And if you look at the actual tests (091 and 085, in this case), the
framework is pretty simple to use. If you run into something
unsupported (json parsing, etc.), then you can use iotests.py.

Jeff

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Stefan Hajnoczi]

On Mon, May 05, 2014 at 11:44:55AM -0400, Jeff Cody wrote:
> On Mon, May 05, 2014 at 05:32:09PM +0200, Kevin Wolf wrote:
> > Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> > > On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > > > This adds some common functionality to control QEMU for qemu-iotests.
> > > > 
> > > > Additionally, test 085 is updated to use this new functionality.
> > > > 
> > > > Some minor fixups along the way, to clear up spaced pathname issues, 
> > > > for common.rc, test 019, and test 086.
> > > > 
> > > > 
> > > > Jeff Cody (3):
> > > >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> > > 
> > > Once a test launches QEMU, it soon needs to parse QMP commands or wait
> > > for QMP events.  That doesn't lend itself to the traditional
> > > qemu-iotests shell model.  That is why iotests.py exists.
> > > 
> > > Shell script is a poor language for test cases that go beyond
> > > pre-defined commands whose output is saved for diffing.  The string
> > > manipulation is clumsy, JSON is not supported, tricks with fifos can
> > > easily deadlock or break when a process terminates unexpectedly, etc.
> > > 
> > > If we go further in the direction of this patch series, we'll duplicate
> > > existing iotests.py code and have complex shell tests that are hard to
> > > extend.  I think it's time to draw the line and convert any test cases
> > > that need to complexity to Python.
> > > 
> > > Why not use iotests.py?
> > 
> > Because it's hard to use. The "compare against reference output" thing
> > is the first thing that you lose with iotests.py, and it's the most
> > useful feature in qemu-iotests.
> > 
> > When a Python test case fails, you get into real debugging. When a shell
> > script test case fails, you usually see immediately from the reference
> > output diff what's wrong.
> > 
> > I accept iotest.py for anything that needs to evaluate QMP return
> > values, reluctantly, because we have nothing better. But that's it, I
> > don't actually _like_ using it.
> >
> 
> I agree with Kevin.  
> 
> Also, for a test framework, having some language duplication is not
> necessarily a *bad* thing.  One, functionality may be tested in
> slightly different ways, due to architectural differences.  Second, it
> gives developers increased flexibility in writing test cases in a
> language they may be more comfortable in - and that hopefully leads to
> an increased number of tests being written.

It's duplicated effort for the test case authors.  It also requires that
people debugging broken tests switch between different frameworks
depending on the test case that's failing.  We already have two types of
test case (simple and QEMU interaction), now a third is being added that
duplicates QEMU interaction.

Regarding the language, you can get comfortable with Python in an
afternoon.  If you already know C++, Java, Perl, or Ruby then it's very
easy to pick up:
https://docs.python.org/2.7/tutorial/index.html

> And if you look at the actual tests (091 and 085, in this case), the
> framework is pretty simple to use. If you run into something
> unsupported (json parsing, etc.), then you can use iotests.py.

You can already do this stuff in iotests.py but you still wrote it in
shell :).  So I'm not sure I buy that.

Stefan

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Fam Zheng]

On Mon, 05/05 17:32, Kevin Wolf wrote:
> Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> > On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > > This adds some common functionality to control QEMU for qemu-iotests.
> > > 
> > > Additionally, test 085 is updated to use this new functionality.
> > > 
> > > Some minor fixups along the way, to clear up spaced pathname issues, 
> > > for common.rc, test 019, and test 086.
> > > 
> > > 
> > > Jeff Cody (3):
> > >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> > 
> > Once a test launches QEMU, it soon needs to parse QMP commands or wait
> > for QMP events.  That doesn't lend itself to the traditional
> > qemu-iotests shell model.  That is why iotests.py exists.
> > 
> > Shell script is a poor language for test cases that go beyond
> > pre-defined commands whose output is saved for diffing.  The string
> > manipulation is clumsy, JSON is not supported, tricks with fifos can
> > easily deadlock or break when a process terminates unexpectedly, etc.
> > 
> > If we go further in the direction of this patch series, we'll duplicate
> > existing iotests.py code and have complex shell tests that are hard to
> > extend.  I think it's time to draw the line and convert any test cases
> > that need to complexity to Python.
> > 
> > Why not use iotests.py?
> 
> Because it's hard to use. The "compare against reference output" thing
> is the first thing that you lose with iotests.py, and it's the most
> useful feature in qemu-iotests.
> 
> When a Python test case fails, you get into real debugging. When a shell
> script test case fails, you usually see immediately from the reference
> output diff what's wrong.
> 

Perhaps we should add more error information into iotests.py, if a test case
fails.

Otherwise I don't think iotests.py hard to use though, out of my subjective
point. I agree that shell is not as goog as python in this kind of test.

Fam

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Stefan Hajnoczi]

On Mon, May 05, 2014 at 05:32:09PM +0200, Kevin Wolf wrote:
> Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> > On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > > This adds some common functionality to control QEMU for qemu-iotests.
> > > 
> > > Additionally, test 085 is updated to use this new functionality.
> > > 
> > > Some minor fixups along the way, to clear up spaced pathname issues, 
> > > for common.rc, test 019, and test 086.
> > > 
> > > 
> > > Jeff Cody (3):
> > >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> > 
> > Once a test launches QEMU, it soon needs to parse QMP commands or wait
> > for QMP events.  That doesn't lend itself to the traditional
> > qemu-iotests shell model.  That is why iotests.py exists.
> > 
> > Shell script is a poor language for test cases that go beyond
> > pre-defined commands whose output is saved for diffing.  The string
> > manipulation is clumsy, JSON is not supported, tricks with fifos can
> > easily deadlock or break when a process terminates unexpectedly, etc.
> > 
> > If we go further in the direction of this patch series, we'll duplicate
> > existing iotests.py code and have complex shell tests that are hard to
> > extend.  I think it's time to draw the line and convert any test cases
> > that need to complexity to Python.
> > 
> > Why not use iotests.py?
> 
> Because it's hard to use. The "compare against reference output" thing
> is the first thing that you lose with iotests.py, and it's the most
> useful feature in qemu-iotests.
> 
> When a Python test case fails, you get into real debugging. When a shell
> script test case fails, you usually see immediately from the reference
> output diff what's wrong.
> 
> I accept iotest.py for anything that needs to evaluate QMP return
> values, reluctantly, because we have nothing better. But that's it, I
> don't actually _like_ using it.

I agree with what you say but we're arguing about different things.

I'm saying:
 * For command output diff tests, use shell.
 * For tests that interact dynamically with running QEMU, use
   iotests.py.

You're saying:
 * Command output diff tests are easy to understand and debug.
 * Tests that interact dynamically with QEMU are harder to debug.

Just because we like the simple shell tests better doesn't mean writing
complex interaction tests in shell will make them simple!

These test cases in this patch aren't simple shell tests.  Let's admit
they are complex and use iotests.py, which is a more appropriate tool
for managing a running QEMU process and interacting with it.

Stefan

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Kevin Wolf]

Am 06.05.2014 um 10:29 hat Stefan Hajnoczi geschrieben:
> On Mon, May 05, 2014 at 05:32:09PM +0200, Kevin Wolf wrote:
> > Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> > > On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > > > This adds some common functionality to control QEMU for qemu-iotests.
> > > > 
> > > > Additionally, test 085 is updated to use this new functionality.
> > > > 
> > > > Some minor fixups along the way, to clear up spaced pathname issues, 
> > > > for common.rc, test 019, and test 086.
> > > > 
> > > > 
> > > > Jeff Cody (3):
> > > >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> > > 
> > > Once a test launches QEMU, it soon needs to parse QMP commands or wait
> > > for QMP events.  That doesn't lend itself to the traditional
> > > qemu-iotests shell model.  That is why iotests.py exists.
> > > 
> > > Shell script is a poor language for test cases that go beyond
> > > pre-defined commands whose output is saved for diffing.  The string
> > > manipulation is clumsy, JSON is not supported, tricks with fifos can
> > > easily deadlock or break when a process terminates unexpectedly, etc.
> > > 
> > > If we go further in the direction of this patch series, we'll duplicate
> > > existing iotests.py code and have complex shell tests that are hard to
> > > extend.  I think it's time to draw the line and convert any test cases
> > > that need to complexity to Python.
> > > 
> > > Why not use iotests.py?
> > 
> > Because it's hard to use. The "compare against reference output" thing
> > is the first thing that you lose with iotests.py, and it's the most
> > useful feature in qemu-iotests.
> > 
> > When a Python test case fails, you get into real debugging. When a shell
> > script test case fails, you usually see immediately from the reference
> > output diff what's wrong.
> > 
> > I accept iotest.py for anything that needs to evaluate QMP return
> > values, reluctantly, because we have nothing better. But that's it, I
> > don't actually _like_ using it.
> 
> I agree with what you say but we're arguing about different things.
> 
> I'm saying:
>  * For command output diff tests, use shell.
>  * For tests that interact dynamically with running QEMU, use
>    iotests.py.

That's a false dichotomy to start with. What about test cases that
interact dynamically with running QEMU _and_ we want to use a command
output diff?

But, what this series is aiming at is not really "dynamic" interaction
at all. It is mostly just sending monitor commands and comparing the
return value to reference output. The only time it looks at the return
value is when it needs to wait for completion of something; and grep is
just fine for something like that.

Just have a look at the test cases that run qemu today. These shell
scripts turn out to be really simple. And as you can see in this series,
even things like migration are easy to do.

> You're saying:
>  * Command output diff tests are easy to understand and debug.
>  * Tests that interact dynamically with QEMU are harder to debug.

No. I'm specifically saying that iotests.py tests are harder to debug.
Even if they don't do much dynamic interaction.

> Just because we like the simple shell tests better doesn't mean writing
> complex interaction tests in shell will make them simple!
> 
> These test cases in this patch aren't simple shell tests.  Let's admit
> they are complex and use iotests.py, which is a more appropriate tool
> for managing a running QEMU process and interacting with it.

What's wrong with the tests in this series? I find them quite easy to
read and work with. But yes, I guess you can actually make them complex
by using iotest.py...

Kevin

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Stefan Hajnoczi]

On Tue, May 06, 2014 at 10:51:19AM +0200, Kevin Wolf wrote:
> Am 06.05.2014 um 10:29 hat Stefan Hajnoczi geschrieben:
> > On Mon, May 05, 2014 at 05:32:09PM +0200, Kevin Wolf wrote:
> > > Am 05.05.2014 um 17:21 hat Stefan Hajnoczi geschrieben:
> > > > On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> > > > > This adds some common functionality to control QEMU for qemu-iotests.
> > > > > 
> > > > > Additionally, test 085 is updated to use this new functionality.
> > > > > 
> > > > > Some minor fixups along the way, to clear up spaced pathname issues, 
> > > > > for common.rc, test 019, and test 086.
> > > > > 
> > > > > 
> > > > > Jeff Cody (3):
> > > > >   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
> > > > 
> > > > Once a test launches QEMU, it soon needs to parse QMP commands or wait
> > > > for QMP events.  That doesn't lend itself to the traditional
> > > > qemu-iotests shell model.  That is why iotests.py exists.
> > > > 
> > > > Shell script is a poor language for test cases that go beyond
> > > > pre-defined commands whose output is saved for diffing.  The string
> > > > manipulation is clumsy, JSON is not supported, tricks with fifos can
> > > > easily deadlock or break when a process terminates unexpectedly, etc.
> > > > 
> > > > If we go further in the direction of this patch series, we'll duplicate
> > > > existing iotests.py code and have complex shell tests that are hard to
> > > > extend.  I think it's time to draw the line and convert any test cases
> > > > that need to complexity to Python.
> > > > 
> > > > Why not use iotests.py?
> > > 
> > > Because it's hard to use. The "compare against reference output" thing
> > > is the first thing that you lose with iotests.py, and it's the most
> > > useful feature in qemu-iotests.
> > > 
> > > When a Python test case fails, you get into real debugging. When a shell
> > > script test case fails, you usually see immediately from the reference
> > > output diff what's wrong.
> > > 
> > > I accept iotest.py for anything that needs to evaluate QMP return
> > > values, reluctantly, because we have nothing better. But that's it, I
> > > don't actually _like_ using it.
> > 
> > I agree with what you say but we're arguing about different things.
> > 
> > I'm saying:
> >  * For command output diff tests, use shell.
> >  * For tests that interact dynamically with running QEMU, use
> >    iotests.py.
> 
> That's a false dichotomy to start with. What about test cases that
> interact dynamically with running QEMU _and_ we want to use a command
> output diff?

Use print and ./check will diff the output.

> > You're saying:
> >  * Command output diff tests are easy to understand and debug.
> >  * Tests that interact dynamically with QEMU are harder to debug.
> 
> No. I'm specifically saying that iotests.py tests are harder to debug.
> Even if they don't do much dynamic interaction.

Not true as mentioned above, use print if you want diffing.

> > Just because we like the simple shell tests better doesn't mean writing
> > complex interaction tests in shell will make them simple!
> > 
> > These test cases in this patch aren't simple shell tests.  Let's admit
> > they are complex and use iotests.py, which is a more appropriate tool
> > for managing a running QEMU process and interacting with it.
> 
> What's wrong with the tests in this series? I find them quite easy to
> read and work with. But yes, I guess you can actually make them complex
> by using iotest.py...

I guess we'll only know by converting the tests and comparing.

Re: [Qemu-devel] [PATCH v5 0/3] Add common QEMU control functionality to qemu-iotests

[Stefan Hajnoczi]

On Wed, Apr 30, 2014 at 10:55:07AM -0400, Jeff Cody wrote:
> Prior cover letter details:
> 
> Changes from v4:
> 
> 
> Patch 1:  - Allow option in _send_qemu_cmd() to not exit on error, similar
>             to _timed_wait_for() (Thanks Fam)
>           - Renamed the option variable to not exit on error, to make
>             it clearer the function.
>           - Updated commit message to reflect the above 2 changes
> 
> Patch 2:  - Fixed typo in commit message (thanks Fam)
>           - Added Fam's reviewed-by
> 
> Patch 3:  - Drop unneeded "cluster_size=512" override from image creation
>             (Thanks Kam)
>           - Write pattern to the image, and verify the pattern (Thanks Fam)
>           - Added Fam's reviewed-by
>            
> 
> 
> Changes from v3:
> 
> Patch 1:   - Make bash version check work with >= 5 as well (Thanks Kevin)
>            - Suppress error message if killing non-existent PID
>            - Close QEMU fifo in/out file descriptors on cleanup (Thanks Kevin)
>            - Remove extra newline at the end of the file (Thanks Kevin)
> 
> Patch 2:    - Update commit message to document the commandline change 
>                 of accel=qtest due to Patch 1 (Thanks Kevin)
> 
> Patch 3:    - Whitespace cleanup (Thanks Kevin)
>             - Cleanly flush and exit QEMU on vm2, and run
>                qemu-img check -r all (Thanks Kevin)   
>             - YATNC - Yet Another Test Number Conflict;
>                 move test from 090 to 091 (Thanks Kevin)
> 
> Patches 4,5:  Already applied to Kevin's block tree
> 
> 
> Changes from v2:
> 
> Updated Reviewed-by for Fam and Benoit (Benoit's from the v1 patch, I forgot to
> add those to v2)
> 
>     Patch 1: * updated commit message (Thanks Fam)
>              * Addded '-machine accel=qtest' to qemu launch args (Thanks Fam)
>     Patch 3: * Moved from test 089 -> test 090 to avoid collision with
>                Fam's series (Thanks Fam)
>     
> Changes from v1:
> 
>     Patch 1: * Fixed commit message, clarified comments (Thanks Benoît)
>              * Changed 'shift' line to be POSIX-friendly, instead of
>                relying on bashism (Thanks Eric)
>              * Added ability to repeat qmp or hmp commands an arbitrary
>                number of times
>     Patch 3: New patch, for live migration
> 
> Original Description:
> 
> This adds some common functionality to control QEMU for qemu-iotests.
> 
> Additionally, test 085 is updated to use this new functionality.
> 
> Some minor fixups along the way, to clear up spaced pathname issues, 
> for common.rc, test 019, and test 086.
> 
> 
> Jeff Cody (3):
>   block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
>   block: qemu-iotests - update 085 to use common.qemu
>   block: qemu-iotests - test for live migration
> 
>  tests/qemu-iotests/085         |  73 +++------------
>  tests/qemu-iotests/091         | 105 ++++++++++++++++++++++
>  tests/qemu-iotests/091.out     |  28 ++++++
>  tests/qemu-iotests/common.qemu | 200 +++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group       |   1 +
>  5 files changed, 346 insertions(+), 61 deletions(-)
>  create mode 100755 tests/qemu-iotests/091
>  create mode 100644 tests/qemu-iotests/091.out
>  create mode 100644 tests/qemu-iotests/common.qemu

Although I think bash is the wrong tool for writing these sorts of tests
(hint: you had to use eval!), let's take this test case.

I will be working on documenting and making iotests.py easier to work
with - Kevin has requested the ability to make the QMP interaction
diffable so tests can be easily debugged.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan