Re: [PATCH 00/67] iotests: Honor $IMGOPTS in Python tests

From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [PATCH 00/67] iotests: Honor $IMGOPTS in Python tests
Date: Tue, 1 Oct 2019 17:47:59 -0400	[thread overview]
Message-ID: <cbb8017a-898b-5658-d176-a76537e3b264@redhat.com> (raw)
In-Reply-To: <20191001194715.2796-1-mreitz@redhat.com>

On 10/1/19 3:46 PM, Max Reitz wrote:
> First of all: Sorry.
> 

Thank you for finding the time to do it.

> 
> Second:
> 
> Based-on: My block branch
>           (https://github.com/XanClic/qemu.git block)
> 
> Based-on: 20190917234549.22910-1-jsnow@redhat.com
>           (“iotests: use python logging”)
> 
> Based-on: 20190927094242.11152-1-mreitz@redhat.com
>           (“iotests: Allow ./check -o data_file”)
> 
> Based-on: 20190917092004.999-1-mreitz@redhat.com
>           (“iotests: Selfish patches”)
> 
> Based-on: 20191001174827.11081-1-mreitz@redhat.com
>           (“block: Skip COR for inactive nodes”)
> 
> 
> OK, now:
> 
> Hi,
> 
> My recent series “iotests: Allow ./check -o data_file” enabled our bash
> tests to interpret the data_file qcow2 option.  It didn’t do anything
> for Python tests because those currently completely ignore all image
> format options.
> 
> This is where it gets hairy.  To do so, we need two things: First of
> all, whatever way Python tests use to create images needs to interpret
> $IMGOPTS.  Second, when deleting image files, they must not use a plain
> os.remove(), but a special function that will clean up data files, too.
> 
> The heap of patches in this series comes from making the Python tests
> use these new functions.
> 
> Most Python tests just run qemu-img through a helper function that does
> not care about the exact subcommand to create images.  I could add
> $IMGOPTS support to it, but that doesn’t feel quite right to me, and it
> wouldn’t reduce the patch count because we still need a special removal
> function.
> 
> 
> This series is structured as follows:
> - Patches 1 through 7 add support to handle image files differently from
>   other files (consider $IMGOPTS when creating them, consider data files
>   when deleting them, separate ImagePaths from FilePaths, and so on)

OK, that makes sense. I suppose we've been playing a bit fast and loose
with these such things.

> 
> - Patches 8 and 9 add two filters we’ll need in the next range:
> 
> - Patches 10 through 13 address some issues in a handful of tests that
>   just need to be changed a little so they can overall work with some
>   format options
> 
> - Patch 14 makes all tests pass unsupported_imgopts where there are
>   options that they cannot support.
> 
> - Patches 15 through 65 make all Python tests use the new functions
>   introduced in the first 7 patches so they no longer ignore $IMGOPTS.
> 
>   I felt like this is much better than munching everything together into
>   a single big commit (better to rebase, better to review), and I don’t
>   really like ideas like “Just do five patches that each address ten
>   iotests”.
> 

This is the right approach, for the exact reasons you specify.

>   But I’m still very much open to suggestions on how to combine these
>   many small patches to reduce the overall patch count.
> 

You could group them by release windows, if you really wanted to;

- [...etc...]
- Update iotests added for 3.2
- Update iotests added for 4.0
- Update iotests added for 4.1
- Individual patches thereafter

But maybe that doesn't really solve anything for anyone. If you didn't
find a more obvious grouping for these, I'd just leave it alone. I'll
get to reviewing them.

> - Patch 66 ensures that Python tests always use the new function to
>   create test images so they won’t bypass $IMGOPTS.
> 
> - Patch 67 cleans up.  qemu_img_log() is only used for image creation,
>   and I don’t see the point in that.  The output is predictable and it
>   is very unlikely to fail.  We can see in the bash tests that regularly
>   we basically just filter everything from it anyway.
>   (So this series replaces log(qemu_img_pipe()) instances by asserting
>   that image creation did not fail.)
>   ((qemu_img_create() obviously no longer has any use after this
>   series.))
> 
> 
> After this series, running the iotests with -o compat=0.10,
> -o refcount_bits=1, and -o 'data_file=$TEST_IMG.data_file' does
> something sensible even for the Python tests, and it passes.
> 

No minor accomplishment.

I'll make sure to review at least 1-14, but not before Friday.

> 
> Max Reitz (67):
>   iotests.py: Read $IMGOPTS
>   iotests.py: Add @skip_for_imgopts()
>   iotests.py: Add unsupported_imgopts
>   iotests.py: create_test_image, remove_test_image
>   iotests.py: Add ImagePaths
>   iotests.py: Add image_path()
>   iotests.py: Filter data_file in filter_img_info
>   iotests.py: Add filter_json_filename()
>   iotests.py: Add @hide_fields to img_info_log
>   iotests/169: Skip persistent cases for compat=0.10
>   iotests/224: Filter json:{} from commit command
>   iotests/228: Filter json:{} filenames
>   iotests/242: Hide refcount bit information
>   iotests: Use unsupported_imgopts in Python tests
>   iotests/030: Honor $IMGOPTS
>   iotests/040: Honor $IMGOPTS
>   iotests/041: Honor $IMGOPTS
>   iotests/044: Honor $IMGOPTS
>   iotests/045: Honor $IMGOPTS
>   iotests/055: Honor $IMGOPTS
>   iotests/056: Honor $IMGOPTS
>   iotests/057: Honor $IMGOPTS
>   iotests/065: Honor $IMGOPTS
>   iotests/096: Honor $IMGOPTS
>   iotests/118: Honor $IMGOPTS
>   iotests/124: Honor $IMGOPTS
>   iotests/129: Honor $IMGOPTS
>   iotests/132: Honor $IMGOPTS
>   iotests/139: Honor $IMGOPTS
>   iotests/147: Honor $IMGOPTS
>   iotests/148: Honor $IMGOPTS
>   iotests/151: Honor $IMGOPTS
>   iotests/152: Honor $IMGOPTS
>   iotests/155: Honor $IMGOPTS
>   iotests/163: Honor $IMGOPTS
>   iotests/165: Honor $IMGOPTS
>   iotests/169: Honor $IMGOPTS
>   iotests/194: Honor $IMGOPTS
>   iotests/196: Honor $IMGOPTS
>   iotests/199: Honor $IMGOPTS
>   iotests/202: Honor $IMGOPTS
>   iotests/203: Honor $IMGOPTS
>   iotests/205: Honor $IMGOPTS
>   iotests/208: Honor $IMGOPTS
>   iotests/208: Honor $IMGOPTS
>   iotests/216: Honor $IMGOPTS
>   iotests/218: Honor $IMGOPTS
>   iotests/219: Honor $IMGOPTS
>   iotests/222: Honor $IMGOPTS
>   iotests/224: Honor $IMGOPTS
>   iotests/228: Honor $IMGOPTS
>   iotests/234: Honor $IMGOPTS
>   iotests/235: Honor $IMGOPTS
>   iotests/236: Honor $IMGOPTS
>   iotests/237: Honor $IMGOPTS
>   iotests/242: Honor $IMGOPTS
>   iotests/245: Honor $IMGOPTS
>   iotests/246: Honor $IMGOPTS
>   iotests/248: Honor $IMGOPTS
>   iotests/254: Honor $IMGOPTS
>   iotests/255: Honor $IMGOPTS
>   iotests/256: Honor $IMGOPTS
>   iotests/257: Honor $IMGOPTS
>   iotests/258: Honor $IMGOPTS
>   iotests/262: Honor $IMGOPTS
>   iotests.py: Forbid qemu_img*('create', ...)
>   iotests.py: Drop qemu_img_log(), qemu_img_create()
> 
>  tests/qemu-iotests/030        |  69 ++++++------
>  tests/qemu-iotests/040        |  42 ++++----
>  tests/qemu-iotests/041        | 108 +++++++++----------
>  tests/qemu-iotests/044        |  11 +-
>  tests/qemu-iotests/045        |  26 ++---
>  tests/qemu-iotests/055        |  41 +++----
>  tests/qemu-iotests/056        |  30 +++---
>  tests/qemu-iotests/057        |  10 +-
>  tests/qemu-iotests/065        |  21 ++--
>  tests/qemu-iotests/096        |   5 +-
>  tests/qemu-iotests/118        |  26 ++---
>  tests/qemu-iotests/124        |  29 +++--
>  tests/qemu-iotests/129        |  11 +-
>  tests/qemu-iotests/132        |   6 +-
>  tests/qemu-iotests/139        |  15 ++-
>  tests/qemu-iotests/147        |  11 +-
>  tests/qemu-iotests/148        |   5 +-
>  tests/qemu-iotests/151        |  10 +-
>  tests/qemu-iotests/152        |   6 +-
>  tests/qemu-iotests/155        |  29 +++--
>  tests/qemu-iotests/163        |  29 ++---
>  tests/qemu-iotests/165        |  10 +-
>  tests/qemu-iotests/169        |  23 ++--
>  tests/qemu-iotests/194        |   9 +-
>  tests/qemu-iotests/196        |  10 +-
>  tests/qemu-iotests/199        |  10 +-
>  tests/qemu-iotests/202        |   9 +-
>  tests/qemu-iotests/203        |   9 +-
>  tests/qemu-iotests/205        |   7 +-
>  tests/qemu-iotests/206        |   5 +-
>  tests/qemu-iotests/208        |   5 +-
>  tests/qemu-iotests/209        |   9 +-
>  tests/qemu-iotests/216        |  11 +-
>  tests/qemu-iotests/218        |   6 +-
>  tests/qemu-iotests/219        |   5 +-
>  tests/qemu-iotests/222        |  13 +--
>  tests/qemu-iotests/224        |  33 +++---
>  tests/qemu-iotests/224.out    |   4 +-
>  tests/qemu-iotests/228        |  25 ++---
>  tests/qemu-iotests/228.out    |   8 +-
>  tests/qemu-iotests/234        |   9 +-
>  tests/qemu-iotests/235        |   7 +-
>  tests/qemu-iotests/236        |   6 +-
>  tests/qemu-iotests/237        |  15 +--
>  tests/qemu-iotests/237.out    |   6 --
>  tests/qemu-iotests/242        |  21 ++--
>  tests/qemu-iotests/242.out    |   5 -
>  tests/qemu-iotests/245        |  21 ++--
>  tests/qemu-iotests/246        |  11 +-
>  tests/qemu-iotests/248        |  14 ++-
>  tests/qemu-iotests/254        |  10 +-
>  tests/qemu-iotests/255        |  20 ++--
>  tests/qemu-iotests/255.out    |   8 --
>  tests/qemu-iotests/256        |  10 +-
>  tests/qemu-iotests/257        |  18 ++--
>  tests/qemu-iotests/258        |  16 +--
>  tests/qemu-iotests/262        |   5 +-
>  tests/qemu-iotests/iotests.py | 197 +++++++++++++++++++++++++++-------
>  58 files changed, 654 insertions(+), 496 deletions(-)
> 