| Message ID | 20191001194715.2796-1-mreitz@redhat.com |
|---|---|
| Headers | show |
| Series | iotests: Honor $IMGOPTS in Python tests | expand |
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(-) >
First of all: Sorry. 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) - 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”. But I’m still very much open to suggestions on how to combine these many small patches to reduce the overall patch count. - 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. 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(-)