| Message ID | 20220221170845.628429-3-hreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Check for zstd support | expand |
On 21/02/2022 18.08, Hanna Reitz wrote: > 303 runs two test cases, one of which requires zstd support. > Unfortunately, given that this is not a unittest-style test, we cannot > easily skip that single case, and instead can only skip the whole test. > > (Alternatively, we could split this test into a zlib and a zstd part, > but that seems excessive, given that this test is not in auto and thus > likely only run by developers who have zstd support compiled in.) > > Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type") > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > tests/qemu-iotests/303 | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303 > index 16c2e10827..5a3efb4ba3 100755 > --- a/tests/qemu-iotests/303 > +++ b/tests/qemu-iotests/303 > @@ -21,7 +21,8 @@ > > import iotests > import subprocess > -from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io > +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, file_path, \ > + log, filter_qemu_io > > iotests.script_initialize(supported_fmts=['qcow2'], > unsupported_imgopts=['refcount_bits', 'compat']) > @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled): > > > def test(compression_type: str, json_output: bool) -> None: > - qemu_img_create('-f', iotests.imgfmt, > - '-o', f'compression_type={compression_type}', > - disk, '10M') > + opts = f'compression_type={compression_type}' > + output, status = qemu_img_pipe_and_status('create', > + '-f', iotests.imgfmt, > + '-o', opts, > + disk, '10M') > + if status == 1 and \ > + "'compression-type' does not accept value 'zstd'" in output: > + notrun('zstd compression not supported') > + > add_bitmap(1, 0, 6, False) > add_bitmap(2, 6, 8, True) > 303 was failing on my system, too, but I did not get around to investigate on that one yet - so thanks for fixing this pro-actively! Now the test is correctly skipped on my system. Tested-by: Thomas Huth <thuth@redhat.com>
21.02.2022 20:08, Hanna Reitz wrote: > 303 runs two test cases, one of which requires zstd support. > Unfortunately, given that this is not a unittest-style test, we cannot > easily skip that single case, and instead can only skip the whole test. > > (Alternatively, we could split this test into a zlib and a zstd part, > but that seems excessive, given that this test is not in auto and thus > likely only run by developers who have zstd support compiled in.) > > Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type") > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > tests/qemu-iotests/303 | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303 > index 16c2e10827..5a3efb4ba3 100755 > --- a/tests/qemu-iotests/303 > +++ b/tests/qemu-iotests/303 > @@ -21,7 +21,8 @@ > > import iotests > import subprocess > -from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io > +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, file_path, \ > + log, filter_qemu_io > > iotests.script_initialize(supported_fmts=['qcow2'], > unsupported_imgopts=['refcount_bits', 'compat']) > @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled): > > > def test(compression_type: str, json_output: bool) -> None: > - qemu_img_create('-f', iotests.imgfmt, > - '-o', f'compression_type={compression_type}', > - disk, '10M') > + opts = f'compression_type={compression_type}' > + output, status = qemu_img_pipe_and_status('create', > + '-f', iotests.imgfmt, > + '-o', opts, > + disk, '10M') > + if status == 1 and \ > + "'compression-type' does not accept value 'zstd'" in output: > + notrun('zstd compression not supported') > + Could we make from it a function in iotests.py like has_working_luks() / verify_working_luks() ? Then in this test we'll simply have at start iotests.verify_zstd(), and in previous test we'll do something like "comp_type = 'zstd' if iotests.has_zstd() else 'zlib'" in classes that wants zstd > add_bitmap(1, 0, 6, False) > add_bitmap(2, 6, 8, True) >
On 22.02.22 16:55, Vladimir Sementsov-Ogievskiy wrote: > 21.02.2022 20:08, Hanna Reitz wrote: >> 303 runs two test cases, one of which requires zstd support. >> Unfortunately, given that this is not a unittest-style test, we cannot >> easily skip that single case, and instead can only skip the whole test. >> >> (Alternatively, we could split this test into a zlib and a zstd part, >> but that seems excessive, given that this test is not in auto and thus >> likely only run by developers who have zstd support compiled in.) >> >> Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type") >> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >> --- >> tests/qemu-iotests/303 | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303 >> index 16c2e10827..5a3efb4ba3 100755 >> --- a/tests/qemu-iotests/303 >> +++ b/tests/qemu-iotests/303 >> @@ -21,7 +21,8 @@ >> import iotests >> import subprocess >> -from iotests import qemu_img_create, qemu_io, file_path, log, >> filter_qemu_io >> +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, >> file_path, \ >> + log, filter_qemu_io >> iotests.script_initialize(supported_fmts=['qcow2'], >> unsupported_imgopts=['refcount_bits', 'compat']) >> @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled): >> def test(compression_type: str, json_output: bool) -> None: >> - qemu_img_create('-f', iotests.imgfmt, >> - '-o', f'compression_type={compression_type}', >> - disk, '10M') >> + opts = f'compression_type={compression_type}' >> + output, status = qemu_img_pipe_and_status('create', >> + '-f', iotests.imgfmt, >> + '-o', opts, >> + disk, '10M') >> + if status == 1 and \ >> + "'compression-type' does not accept value 'zstd'" in >> output: >> + notrun('zstd compression not supported') >> + > > Could we make from it a function in iotests.py like has_working_luks() > / verify_working_luks() ? > > Then in this test we'll simply have at start iotests.verify_zstd(), > and in previous test we'll do something like "comp_type = 'zstd' if > iotests.has_zstd() else 'zlib'" in classes that wants zstd Well, to be honest, I deliberately didn’t do that. We have qemu-img create calls in both of these places, where we can simply check the returned error string for free, basically. Adding a function for this check doesn’t really reduce complexity (now), because adding such functions probably has some more LoC than just checking the returned error string in these two places (I say “now” because it’s possible that more places are added in the future, though, of course). With zstd support compiled in, such functions would effectively also just add a superfluous qemu-img create call wherever they’re used. I’m not saying I’m right about that judgment, because it does feel a bit like premature optimization. Just saying why I didn’t add such functions. So, with that said, would you still prefer such iotests.py functions? I’ll absolutely add them in v2 if you do. Hanna
02.03.2022 14:09, Hanna Reitz wrote: > On 22.02.22 16:55, Vladimir Sementsov-Ogievskiy wrote: >> 21.02.2022 20:08, Hanna Reitz wrote: >>> 303 runs two test cases, one of which requires zstd support. >>> Unfortunately, given that this is not a unittest-style test, we cannot >>> easily skip that single case, and instead can only skip the whole test. >>> >>> (Alternatively, we could split this test into a zlib and a zstd part, >>> but that seems excessive, given that this test is not in auto and thus >>> likely only run by developers who have zstd support compiled in.) >>> >>> Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type") >>> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >>> --- >>> tests/qemu-iotests/303 | 15 +++++++++++---- >>> 1 file changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303 >>> index 16c2e10827..5a3efb4ba3 100755 >>> --- a/tests/qemu-iotests/303 >>> +++ b/tests/qemu-iotests/303 >>> @@ -21,7 +21,8 @@ >>> import iotests >>> import subprocess >>> -from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io >>> +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, file_path, \ >>> + log, filter_qemu_io >>> iotests.script_initialize(supported_fmts=['qcow2'], >>> unsupported_imgopts=['refcount_bits', 'compat']) >>> @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled): >>> def test(compression_type: str, json_output: bool) -> None: >>> - qemu_img_create('-f', iotests.imgfmt, >>> - '-o', f'compression_type={compression_type}', >>> - disk, '10M') >>> + opts = f'compression_type={compression_type}' >>> + output, status = qemu_img_pipe_and_status('create', >>> + '-f', iotests.imgfmt, >>> + '-o', opts, >>> + disk, '10M') >>> + if status == 1 and \ >>> + "'compression-type' does not accept value 'zstd'" in output: >>> + notrun('zstd compression not supported') >>> + >> >> Could we make from it a function in iotests.py like has_working_luks() / verify_working_luks() ? >> >> Then in this test we'll simply have at start iotests.verify_zstd(), and in previous test we'll do something like "comp_type = 'zstd' if iotests.has_zstd() else 'zlib'" in classes that wants zstd > > Well, to be honest, I deliberately didn’t do that. We have qemu-img create calls in both of these places, where we can simply check the returned error string for free, basically. Adding a function for this check doesn’t really reduce complexity (now), because adding such functions probably has some more LoC than just checking the returned error string in these two places (I say “now” because it’s possible that more places are added in the future, though, of course). With zstd support compiled in, such functions would effectively also just add a superfluous qemu-img create call wherever they’re used. > > I’m not saying I’m right about that judgment, because it does feel a bit like premature optimization. Just saying why I didn’t add such functions. > > So, with that said, would you still prefer such iotests.py functions? I’ll absolutely add them in v2 if you do. > Yes, I think better is to have generic function for such thing: 1. These modifications makes tests more complicated to read (OK only a bit more complicated, but still). And they break common practice that, necessary features are controlled by verify_* functions, parameters to iotests.main and so on. 2. I understand your doubt that we just do an extra qemu_img call. But for sure there are solutions if we want: - At least, we can call verify_* function once and cache its result for the whole test run. - More, we can group several verify_* things into one qemu process call, to do different checks by qmp commands in context of that one qemu process test run, before running real tests. - And about zstd support - we can even generate a file with some environment variables as we do before (remember tests/qemu-iotests/common.env.in), and meson will set a CONFIG_ZSTD variable in it, and then in iotests.py we'll know, do we have zstd support without any extra qemu-img call. Hmm, and probably when we run tests through make check, we already have this information (I mean CONFIG_ZSTD).
On 02.03.22 12:31, Vladimir Sementsov-Ogievskiy wrote: > 02.03.2022 14:09, Hanna Reitz wrote: >> On 22.02.22 16:55, Vladimir Sementsov-Ogievskiy wrote: >>> 21.02.2022 20:08, Hanna Reitz wrote: >>>> 303 runs two test cases, one of which requires zstd support. >>>> Unfortunately, given that this is not a unittest-style test, we cannot >>>> easily skip that single case, and instead can only skip the whole >>>> test. >>>> >>>> (Alternatively, we could split this test into a zlib and a zstd part, >>>> but that seems excessive, given that this test is not in auto and thus >>>> likely only run by developers who have zstd support compiled in.) >>>> >>>> Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression >>>> type") >>>> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >>>> --- >>>> tests/qemu-iotests/303 | 15 +++++++++++---- >>>> 1 file changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303 >>>> index 16c2e10827..5a3efb4ba3 100755 >>>> --- a/tests/qemu-iotests/303 >>>> +++ b/tests/qemu-iotests/303 >>>> @@ -21,7 +21,8 @@ >>>> import iotests >>>> import subprocess >>>> -from iotests import qemu_img_create, qemu_io, file_path, log, >>>> filter_qemu_io >>>> +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, >>>> file_path, \ >>>> + log, filter_qemu_io >>>> iotests.script_initialize(supported_fmts=['qcow2'], >>>> unsupported_imgopts=['refcount_bits', 'compat']) >>>> @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled): >>>> def test(compression_type: str, json_output: bool) -> None: >>>> - qemu_img_create('-f', iotests.imgfmt, >>>> - '-o', f'compression_type={compression_type}', >>>> - disk, '10M') >>>> + opts = f'compression_type={compression_type}' >>>> + output, status = qemu_img_pipe_and_status('create', >>>> + '-f', iotests.imgfmt, >>>> + '-o', opts, >>>> + disk, '10M') >>>> + if status == 1 and \ >>>> + "'compression-type' does not accept value 'zstd'" in >>>> output: >>>> + notrun('zstd compression not supported') >>>> + >>> >>> Could we make from it a function in iotests.py like >>> has_working_luks() / verify_working_luks() ? >>> >>> Then in this test we'll simply have at start iotests.verify_zstd(), >>> and in previous test we'll do something like "comp_type = 'zstd' if >>> iotests.has_zstd() else 'zlib'" in classes that wants zstd >> >> Well, to be honest, I deliberately didn’t do that. We have qemu-img >> create calls in both of these places, where we can simply check the >> returned error string for free, basically. Adding a function for this >> check doesn’t really reduce complexity (now), because adding such >> functions probably has some more LoC than just checking the returned >> error string in these two places (I say “now” because it’s possible >> that more places are added in the future, though, of course). With >> zstd support compiled in, such functions would effectively also just >> add a superfluous qemu-img create call wherever they’re used. >> >> I’m not saying I’m right about that judgment, because it does feel a >> bit like premature optimization. Just saying why I didn’t add such >> functions. >> >> So, with that said, would you still prefer such iotests.py functions? >> I’ll absolutely add them in v2 if you do. >> > > Yes, I think better is to have generic function for such thing: > > 1. These modifications makes tests more complicated to read (OK only a > bit more complicated, but still). And they break common practice that, > necessary features are controlled by verify_* functions, parameters to > iotests.main and so on. OK. > 2. I understand your doubt that we just do an extra qemu_img call. But > for sure there are solutions if we want: > > - At least, we can call verify_* function once and cache its result > for the whole test run. > - More, we can group several verify_* things into one qemu process > call, to do different checks by qmp commands in context of that one > qemu process test run, before running real tests. > - And about zstd support - we can even generate a file with some > environment variables as we do before (remember > tests/qemu-iotests/common.env.in), and meson will set a CONFIG_ZSTD > variable in it, and then in iotests.py we'll know, do we have zstd > support without any extra qemu-img call. Hmm, and probably when we run > tests through make check, we already have this information (I mean > CONFIG_ZSTD). I thought of similar stuff, but that seems so complicated that it kind of breaks argument 1. Caching in iotests.py is simple enough[1], but caching across different test instances isn’t really. [1] Well. If I add it to has_qcow2_zstd_compression(), then the question will arise why I’m not adding it to has_working_luks() also. So that way, just caching in iotests.py would make this series already more complicated than it was here in v1. Which is why I’d rather not add any caching at all.
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303 index 16c2e10827..5a3efb4ba3 100755 --- a/tests/qemu-iotests/303 +++ b/tests/qemu-iotests/303 @@ -21,7 +21,8 @@ import iotests import subprocess -from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, file_path, \ + log, filter_qemu_io iotests.script_initialize(supported_fmts=['qcow2'], unsupported_imgopts=['refcount_bits', 'compat']) @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled): def test(compression_type: str, json_output: bool) -> None: - qemu_img_create('-f', iotests.imgfmt, - '-o', f'compression_type={compression_type}', - disk, '10M') + opts = f'compression_type={compression_type}' + output, status = qemu_img_pipe_and_status('create', + '-f', iotests.imgfmt, + '-o', opts, + disk, '10M') + if status == 1 and \ + "'compression-type' does not accept value 'zstd'" in output: + notrun('zstd compression not supported') + add_bitmap(1, 0, 6, False) add_bitmap(2, 6, 8, True)
303 runs two test cases, one of which requires zstd support. Unfortunately, given that this is not a unittest-style test, we cannot easily skip that single case, and instead can only skip the whole test. (Alternatively, we could split this test into a zlib and a zstd part, but that seems excessive, given that this test is not in auto and thus likely only run by developers who have zstd support compiled in.) Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type") Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/303 | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)