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(-)
>
prev parent reply other threads:[~2019-10-01 21:58 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-01 19:46 [PATCH 00/67] iotests: Honor $IMGOPTS in Python tests Max Reitz
2019-10-01 19:46 ` [PATCH 01/67] iotests.py: Read $IMGOPTS Max Reitz
2019-10-01 22:16 ` John Snow
2019-10-03 15:03 ` Vladimir Sementsov-Ogievskiy
2019-10-03 15:08 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:46 ` [PATCH 02/67] iotests.py: Add @skip_for_imgopts() Max Reitz
2019-10-01 22:16 ` John Snow
2019-10-03 15:19 ` Vladimir Sementsov-Ogievskiy
2019-10-04 12:55 ` Max Reitz
2019-10-01 19:46 ` [PATCH 03/67] iotests.py: Add unsupported_imgopts Max Reitz
2019-10-01 22:18 ` John Snow
2019-10-03 15:21 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:46 ` [PATCH 04/67] iotests.py: create_test_image, remove_test_image Max Reitz
2019-10-01 23:20 ` John Snow
2019-10-01 23:30 ` John Snow
2019-10-02 11:00 ` Max Reitz
2019-10-03 0:35 ` John Snow
2019-10-01 19:46 ` [PATCH 05/67] iotests.py: Add ImagePaths Max Reitz
2019-10-01 19:46 ` [PATCH 06/67] iotests.py: Add image_path() Max Reitz
2019-10-01 19:46 ` [PATCH 07/67] iotests.py: Filter data_file in filter_img_info Max Reitz
2019-10-01 19:46 ` [PATCH 08/67] iotests.py: Add filter_json_filename() Max Reitz
2019-10-01 19:46 ` [PATCH 09/67] iotests.py: Add @hide_fields to img_info_log Max Reitz
2019-10-01 19:46 ` [PATCH 10/67] iotests/169: Skip persistent cases for compat=0.10 Max Reitz
2019-10-01 19:46 ` [PATCH 11/67] iotests/224: Filter json:{} from commit command Max Reitz
2019-10-01 19:46 ` [PATCH 12/67] iotests/228: Filter json:{} filenames Max Reitz
2019-10-01 19:46 ` [PATCH 13/67] iotests/242: Hide refcount bit information Max Reitz
2019-10-01 19:46 ` [PATCH 14/67] iotests: Use unsupported_imgopts in Python tests Max Reitz
2019-10-01 19:46 ` [PATCH 15/67] iotests/030: Honor $IMGOPTS Max Reitz
2019-10-01 19:46 ` [PATCH 16/67] iotests/040: " Max Reitz
2019-10-01 19:46 ` [PATCH 17/67] iotests/041: " Max Reitz
2019-10-01 19:46 ` [PATCH 18/67] iotests/044: " Max Reitz
2019-10-01 19:46 ` [PATCH 19/67] iotests/045: " Max Reitz
2019-10-01 19:46 ` [PATCH 20/67] iotests/055: " Max Reitz
2019-10-01 19:46 ` [PATCH 21/67] iotests/056: " Max Reitz
2019-10-01 19:46 ` [PATCH 22/67] iotests/057: " Max Reitz
2019-10-01 19:46 ` [PATCH 23/67] iotests/065: " Max Reitz
2019-10-01 19:46 ` [PATCH 24/67] iotests/096: " Max Reitz
2019-10-01 19:46 ` [PATCH 25/67] iotests/118: " Max Reitz
2019-10-01 19:46 ` [PATCH 26/67] iotests/124: " Max Reitz
2019-10-01 19:46 ` [PATCH 27/67] iotests/129: " Max Reitz
2019-10-01 19:46 ` [PATCH 28/67] iotests/132: " Max Reitz
2019-10-01 19:46 ` [PATCH 29/67] iotests/139: " Max Reitz
2019-10-01 19:46 ` [PATCH 30/67] iotests/147: " Max Reitz
2019-10-01 19:46 ` [PATCH 31/67] iotests/148: " Max Reitz
2019-10-01 19:46 ` [PATCH 32/67] iotests/151: " Max Reitz
2019-10-01 19:46 ` [PATCH 33/67] iotests/152: " Max Reitz
2019-10-01 19:46 ` [PATCH 34/67] iotests/155: " Max Reitz
2019-10-01 19:46 ` [PATCH 35/67] iotests/163: " Max Reitz
2019-10-01 19:46 ` [PATCH 36/67] iotests/165: " Max Reitz
2019-10-01 19:46 ` [PATCH 37/67] iotests/169: " Max Reitz
2019-10-01 19:46 ` [PATCH 38/67] iotests/194: " Max Reitz
2019-10-01 19:46 ` [PATCH 39/67] iotests/196: " Max Reitz
2019-10-01 19:46 ` [PATCH 40/67] iotests/199: " Max Reitz
2019-10-01 19:46 ` [PATCH 41/67] iotests/202: " Max Reitz
2019-10-01 19:46 ` [PATCH 42/67] iotests/203: " Max Reitz
2019-10-01 19:46 ` [PATCH 43/67] iotests/205: " Max Reitz
2019-10-01 19:46 ` [PATCH 44/67] iotests/208: " Max Reitz
2019-10-01 19:46 ` [PATCH 45/67] " Max Reitz
2019-10-01 19:46 ` [PATCH 46/67] iotests/216: " Max Reitz
2019-10-01 19:46 ` [PATCH 47/67] iotests/218: " Max Reitz
2019-10-01 19:46 ` [PATCH 48/67] iotests/219: " Max Reitz
2019-10-01 19:46 ` [PATCH 49/67] iotests/222: " Max Reitz
2019-10-01 19:46 ` [PATCH 50/67] iotests/224: " Max Reitz
2019-10-01 19:46 ` [PATCH 51/67] iotests/228: " Max Reitz
2019-10-01 19:47 ` [PATCH 52/67] iotests/234: " Max Reitz
2019-10-01 19:47 ` [PATCH 53/67] iotests/235: " Max Reitz
2019-10-01 19:47 ` [PATCH 54/67] iotests/236: " Max Reitz
2019-10-01 19:47 ` [PATCH 55/67] iotests/237: " Max Reitz
2019-10-01 19:47 ` [PATCH 56/67] iotests/242: " Max Reitz
2019-10-01 19:47 ` [PATCH 57/67] iotests/245: " Max Reitz
2019-10-01 19:47 ` [PATCH 58/67] iotests/246: " Max Reitz
2019-10-01 19:47 ` [PATCH 59/67] iotests/248: " Max Reitz
2019-10-01 19:47 ` [PATCH 60/67] iotests/254: " Max Reitz
2019-10-01 19:47 ` [PATCH 61/67] iotests/255: " Max Reitz
2019-10-01 19:47 ` [PATCH 62/67] iotests/256: " Max Reitz
2019-10-01 19:47 ` [PATCH 63/67] iotests/257: " Max Reitz
2019-10-01 19:47 ` [PATCH 64/67] iotests/258: " Max Reitz
2019-10-01 19:47 ` [PATCH 65/67] iotests/262: " Max Reitz
2019-10-01 19:47 ` [PATCH 66/67] iotests.py: Forbid qemu_img*('create', ...) Max Reitz
2019-10-01 19:47 ` [PATCH 67/67] iotests.py: Drop qemu_img_log(), qemu_img_create() Max Reitz
2019-10-01 21:47 ` John Snow [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cbb8017a-898b-5658-d176-a76537e3b264@redhat.com \
--to=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).