| Message ID | 20190927094242.11152-2-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Allow ./check -o data_file | expand |
On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > This test can run just fine with other values for refcount_bits, so we > should filter the value from qcow2.py's dump-header. > > (036 currently ignores user-specified image options, but that will be > fixed in the next patch.) > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/036 | 9 ++++++--- > tests/qemu-iotests/036.out | 6 +++--- > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 > index f06ff67408..69d0f9f903 100755 > --- a/tests/qemu-iotests/036 > +++ b/tests/qemu-iotests/036 > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 > > # Without feature table > $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 > -$PYTHON qcow2.py "$TEST_IMG" dump-header > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > _img_info > > # With feature table containing bit 63 > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === > echo > _make_test_img 64M > $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 > -$PYTHON qcow2.py "$TEST_IMG" dump-header > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > echo > echo === Repair image === > echo > _check_test_img -r all > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > # success, all done > echo "*** done" > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out > index e489b44386..998c2a8a35 100644 > --- a/tests/qemu-iotests/036.out > +++ b/tests/qemu-iotests/036.out > @@ -19,7 +19,7 @@ snapshot_offset 0x0 > incompatible_features 0x8000000000000000 > compatible_features 0x0 > autoclear_features 0x0 > -refcount_order 4 > +refcount_order (filtered) > header_length 104 > > qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 > @@ -53,7 +53,7 @@ snapshot_offset 0x0 > incompatible_features 0x0 > compatible_features 0x0 > autoclear_features 0x8000000000000000 > -refcount_order 4 > +refcount_order (filtered) > header_length 104 > > Header extension: > @@ -81,7 +81,7 @@ snapshot_offset 0x0 > incompatible_features 0x0 > compatible_features 0x0 > autoclear_features 0x0 > -refcount_order 4 > +refcount_order (filtered) > header_length 104 > > Header extension: Minor notes: 1. Maybe put the sed command into a function to avoid duplication? 2. As I understand the test, it only checks the feature bits. So maybe instead of blacklisting some of the values, white list only the feature bits in the output? Best regards, Maxim Levitsky
On 29.09.19 18:31, Maxim Levitsky wrote: > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: >> This test can run just fine with other values for refcount_bits, so we >> should filter the value from qcow2.py's dump-header. >> >> (036 currently ignores user-specified image options, but that will be >> fixed in the next patch.) >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/036 | 9 ++++++--- >> tests/qemu-iotests/036.out | 6 +++--- >> 2 files changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 >> index f06ff67408..69d0f9f903 100755 >> --- a/tests/qemu-iotests/036 >> +++ b/tests/qemu-iotests/036 >> @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 >> >> # Without feature table >> $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 >> -$PYTHON qcow2.py "$TEST_IMG" dump-header >> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >> _img_info >> >> # With feature table containing bit 63 >> @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === >> echo >> _make_test_img 64M >> $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 >> -$PYTHON qcow2.py "$TEST_IMG" dump-header >> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >> >> echo >> echo === Repair image === >> echo >> _check_test_img -r all >> >> -$PYTHON qcow2.py "$TEST_IMG" dump-header >> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >> >> # success, all done >> echo "*** done" >> diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out >> index e489b44386..998c2a8a35 100644 >> --- a/tests/qemu-iotests/036.out >> +++ b/tests/qemu-iotests/036.out >> @@ -19,7 +19,7 @@ snapshot_offset 0x0 >> incompatible_features 0x8000000000000000 >> compatible_features 0x0 >> autoclear_features 0x0 >> -refcount_order 4 >> +refcount_order (filtered) >> header_length 104 >> >> qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 >> @@ -53,7 +53,7 @@ snapshot_offset 0x0 >> incompatible_features 0x0 >> compatible_features 0x0 >> autoclear_features 0x8000000000000000 >> -refcount_order 4 >> +refcount_order (filtered) >> header_length 104 >> >> Header extension: >> @@ -81,7 +81,7 @@ snapshot_offset 0x0 >> incompatible_features 0x0 >> compatible_features 0x0 >> autoclear_features 0x0 >> -refcount_order 4 >> +refcount_order (filtered) >> header_length 104 >> >> Header extension: > > Minor notes: > > 1. Maybe put the sed command into a function to avoid duplication? Hm, maybe, but that would increase the LoC, so I’m not sure whether it really would be a simplification. > 2. As I understand the test, it only checks the feature bits. > So maybe instead of blacklisting some of the values, white list > only the feature bits in the output? Hm. Seems reasonable, but then again I’d prefer a minimal change, and changing it to a whitelist would be more change... Max
On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote: > On 29.09.19 18:31, Maxim Levitsky wrote: > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > > > This test can run just fine with other values for refcount_bits, so we > > > should filter the value from qcow2.py's dump-header. > > > > > > (036 currently ignores user-specified image options, but that will be > > > fixed in the next patch.) > > > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > > --- > > > tests/qemu-iotests/036 | 9 ++++++--- > > > tests/qemu-iotests/036.out | 6 +++--- > > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 > > > index f06ff67408..69d0f9f903 100755 > > > --- a/tests/qemu-iotests/036 > > > +++ b/tests/qemu-iotests/036 > > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 > > > > > > # Without feature table > > > $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > _img_info > > > > > > # With feature table containing bit 63 > > > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === > > > echo > > > _make_test_img 64M > > > $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > echo > > > echo === Repair image === > > > echo > > > _check_test_img -r all > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > # success, all done > > > echo "*** done" > > > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out > > > index e489b44386..998c2a8a35 100644 > > > --- a/tests/qemu-iotests/036.out > > > +++ b/tests/qemu-iotests/036.out > > > @@ -19,7 +19,7 @@ snapshot_offset 0x0 > > > incompatible_features 0x8000000000000000 > > > compatible_features 0x0 > > > autoclear_features 0x0 > > > -refcount_order 4 > > > +refcount_order (filtered) > > > header_length 104 > > > > > > qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 > > > @@ -53,7 +53,7 @@ snapshot_offset 0x0 > > > incompatible_features 0x0 > > > compatible_features 0x0 > > > autoclear_features 0x8000000000000000 > > > -refcount_order 4 > > > +refcount_order (filtered) > > > header_length 104 > > > > > > Header extension: > > > @@ -81,7 +81,7 @@ snapshot_offset 0x0 > > > incompatible_features 0x0 > > > compatible_features 0x0 > > > autoclear_features 0x0 > > > -refcount_order 4 > > > +refcount_order (filtered) > > > header_length 104 > > > > > > Header extension: > > > > Minor notes: > > > > 1. Maybe put the sed command into a function to avoid duplication? > > Hm, maybe, but that would increase the LoC, so I’m not sure whether it > really would be a simplification. IMHO, in my opinion, regardless of LOC, duplicated code is almost always bad. Common functions are mostly for the next guy that will be able to use them instead of searching through code to see how this is done there and there. > > > 2. As I understand the test, it only checks the feature bits. > > So maybe instead of blacklisting some of the values, white list > > only the feature bits in the output? > > Hm. Seems reasonable, but then again I’d prefer a minimal change, and > changing it to a whitelist would be more change... I don't think this is bad, again in my eyes, the code should be as friendly as possible for the next guy that will have to change it, so adding some more extra changes don't seem a problem to me. Of course this is only my personal option and each approach has its own cons, and pros. This doesn't matter that much to me. Best regards, Maxim Levitsky
On 30.09.19 15:40, Maxim Levitsky wrote: > On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote: >> On 29.09.19 18:31, Maxim Levitsky wrote: >>> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: >>>> This test can run just fine with other values for refcount_bits, so we >>>> should filter the value from qcow2.py's dump-header. >>>> >>>> (036 currently ignores user-specified image options, but that will be >>>> fixed in the next patch.) >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> tests/qemu-iotests/036 | 9 ++++++--- >>>> tests/qemu-iotests/036.out | 6 +++--- >>>> 2 files changed, 9 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 >>>> index f06ff67408..69d0f9f903 100755 >>>> --- a/tests/qemu-iotests/036 >>>> +++ b/tests/qemu-iotests/036 >>>> @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 >>>> >>>> # Without feature table >>>> $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 >>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header >>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >>>> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >>>> _img_info >>>> >>>> # With feature table containing bit 63 >>>> @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === >>>> echo >>>> _make_test_img 64M >>>> $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 >>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header >>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >>>> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >>>> >>>> echo >>>> echo === Repair image === >>>> echo >>>> _check_test_img -r all >>>> >>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header >>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >>>> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >>>> >>>> # success, all done >>>> echo "*** done" >>>> diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out >>>> index e489b44386..998c2a8a35 100644 >>>> --- a/tests/qemu-iotests/036.out >>>> +++ b/tests/qemu-iotests/036.out >>>> @@ -19,7 +19,7 @@ snapshot_offset 0x0 >>>> incompatible_features 0x8000000000000000 >>>> compatible_features 0x0 >>>> autoclear_features 0x0 >>>> -refcount_order 4 >>>> +refcount_order (filtered) >>>> header_length 104 >>>> >>>> qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 >>>> @@ -53,7 +53,7 @@ snapshot_offset 0x0 >>>> incompatible_features 0x0 >>>> compatible_features 0x0 >>>> autoclear_features 0x8000000000000000 >>>> -refcount_order 4 >>>> +refcount_order (filtered) >>>> header_length 104 >>>> >>>> Header extension: >>>> @@ -81,7 +81,7 @@ snapshot_offset 0x0 >>>> incompatible_features 0x0 >>>> compatible_features 0x0 >>>> autoclear_features 0x0 >>>> -refcount_order 4 >>>> +refcount_order (filtered) >>>> header_length 104 >>>> >>>> Header extension: >>> >>> Minor notes: >>> >>> 1. Maybe put the sed command into a function to avoid duplication? >> >> Hm, maybe, but that would increase the LoC, so I’m not sure whether it >> really would be a simplification. > IMHO, in my opinion, regardless of LOC, duplicated code is almost always > bad. Common functions are mostly for the next guy that will be able to use > them instead of searching through code to see how this is done there and there. In my experience, people write tests without scanning common.filter for whether anyone has written the same code already. See the _filter_qemu_img_check example in this series. >> >>> 2. As I understand the test, it only checks the feature bits. >>> So maybe instead of blacklisting some of the values, white list >>> only the feature bits in the output? >> >> Hm. Seems reasonable, but then again I’d prefer a minimal change, and >> changing it to a whitelist would be more change... > I don't think this is bad, again in my eyes, the code should be as friendly > as possible for the next guy that will have to change it, so adding > some more extra changes don't seem a problem to me. In my eyes tests aren’t code. Max > Of course this is only my personal option and each approach has its own cons, > and pros. This doesn't matter that much to me. > > Best regards, > Maxim Levitsky >
On Mon, 2019-09-30 at 15:44 +0200, Max Reitz wrote: > On 30.09.19 15:40, Maxim Levitsky wrote: > > On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote: > > > On 29.09.19 18:31, Maxim Levitsky wrote: > > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > > > > > This test can run just fine with other values for refcount_bits, so we > > > > > should filter the value from qcow2.py's dump-header. > > > > > > > > > > (036 currently ignores user-specified image options, but that will be > > > > > fixed in the next patch.) > > > > > > > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > > > > --- > > > > > tests/qemu-iotests/036 | 9 ++++++--- > > > > > tests/qemu-iotests/036.out | 6 +++--- > > > > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 > > > > > index f06ff67408..69d0f9f903 100755 > > > > > --- a/tests/qemu-iotests/036 > > > > > +++ b/tests/qemu-iotests/036 > > > > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 > > > > > > > > > > # Without feature table > > > > > $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > _img_info > > > > > > > > > > # With feature table containing bit 63 > > > > > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === > > > > > echo > > > > > _make_test_img 64M > > > > > $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > > > > > echo > > > > > echo === Repair image === > > > > > echo > > > > > _check_test_img -r all > > > > > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > > > > > # success, all done > > > > > echo "*** done" > > > > > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out > > > > > index e489b44386..998c2a8a35 100644 > > > > > --- a/tests/qemu-iotests/036.out > > > > > +++ b/tests/qemu-iotests/036.out > > > > > @@ -19,7 +19,7 @@ snapshot_offset 0x0 > > > > > incompatible_features 0x8000000000000000 > > > > > compatible_features 0x0 > > > > > autoclear_features 0x0 > > > > > -refcount_order 4 > > > > > +refcount_order (filtered) > > > > > header_length 104 > > > > > > > > > > qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 > > > > > @@ -53,7 +53,7 @@ snapshot_offset 0x0 > > > > > incompatible_features 0x0 > > > > > compatible_features 0x0 > > > > > autoclear_features 0x8000000000000000 > > > > > -refcount_order 4 > > > > > +refcount_order (filtered) > > > > > header_length 104 > > > > > > > > > > Header extension: > > > > > @@ -81,7 +81,7 @@ snapshot_offset 0x0 > > > > > incompatible_features 0x0 > > > > > compatible_features 0x0 > > > > > autoclear_features 0x0 > > > > > -refcount_order 4 > > > > > +refcount_order (filtered) > > > > > header_length 104 > > > > > > > > > > Header extension: > > > > > > > > Minor notes: > > > > > > > > 1. Maybe put the sed command into a function to avoid duplication? > > > > > > Hm, maybe, but that would increase the LoC, so I’m not sure whether it > > > really would be a simplification. > > > > IMHO, in my opinion, regardless of LOC, duplicated code is almost always > > bad. Common functions are mostly for the next guy that will be able to use > > them instead of searching through code to see how this is done there and there. > > In my experience, people write tests without scanning common.filter for > whether anyone has written the same code already. See the > _filter_qemu_img_check example in this series. Fair enough, but this can change. > > > > > > > > 2. As I understand the test, it only checks the feature bits. > > > > So maybe instead of blacklisting some of the values, white list > > > > only the feature bits in the output? > > > > > > Hm. Seems reasonable, but then again I’d prefer a minimal change, and > > > changing it to a whitelist would be more change... > > > > I don't think this is bad, again in my eyes, the code should be as friendly > > as possible for the next guy that will have to change it, so adding > > some more extra changes don't seem a problem to me. > > In my eyes tests aren’t code. And yet, writing tests is commonly known as a task that developers don't really like to do, so making that even a tiniest bit easier, is a good thing IMHO, Anyway I won't argue about this too much, and on top of this, I think that this patch series does *lot* of improvements that do make it easier to write the tests. Thanks for that, and I might also someday in the future do some refactoring for the iotests. The thing that I hate the most is that the tests don't have names.... Best regards, Maxim Levitsky
On 30.09.19 15:58, Maxim Levitsky wrote: > On Mon, 2019-09-30 at 15:44 +0200, Max Reitz wrote: >> On 30.09.19 15:40, Maxim Levitsky wrote: >>> On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote: >>>> On 29.09.19 18:31, Maxim Levitsky wrote: >>>>> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: >>>>>> This test can run just fine with other values for refcount_bits, so we >>>>>> should filter the value from qcow2.py's dump-header. >>>>>> >>>>>> (036 currently ignores user-specified image options, but that will be >>>>>> fixed in the next patch.) >>>>>> >>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>>>> --- >>>>>> tests/qemu-iotests/036 | 9 ++++++--- >>>>>> tests/qemu-iotests/036.out | 6 +++--- >>>>>> 2 files changed, 9 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 >>>>>> index f06ff67408..69d0f9f903 100755 >>>>>> --- a/tests/qemu-iotests/036 >>>>>> +++ b/tests/qemu-iotests/036 >>>>>> @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 >>>>>> >>>>>> # Without feature table >>>>>> $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 >>>>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header >>>>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >>>>>> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >>>>>> _img_info >>>>>> >>>>>> # With feature table containing bit 63 >>>>>> @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === >>>>>> echo >>>>>> _make_test_img 64M >>>>>> $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 >>>>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header >>>>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >>>>>> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >>>>>> >>>>>> echo >>>>>> echo === Repair image === >>>>>> echo >>>>>> _check_test_img -r all >>>>>> >>>>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header >>>>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \ >>>>>> + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' >>>>>> >>>>>> # success, all done >>>>>> echo "*** done" >>>>>> diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out >>>>>> index e489b44386..998c2a8a35 100644 >>>>>> --- a/tests/qemu-iotests/036.out >>>>>> +++ b/tests/qemu-iotests/036.out >>>>>> @@ -19,7 +19,7 @@ snapshot_offset 0x0 >>>>>> incompatible_features 0x8000000000000000 >>>>>> compatible_features 0x0 >>>>>> autoclear_features 0x0 >>>>>> -refcount_order 4 >>>>>> +refcount_order (filtered) >>>>>> header_length 104 >>>>>> >>>>>> qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 >>>>>> @@ -53,7 +53,7 @@ snapshot_offset 0x0 >>>>>> incompatible_features 0x0 >>>>>> compatible_features 0x0 >>>>>> autoclear_features 0x8000000000000000 >>>>>> -refcount_order 4 >>>>>> +refcount_order (filtered) >>>>>> header_length 104 >>>>>> >>>>>> Header extension: >>>>>> @@ -81,7 +81,7 @@ snapshot_offset 0x0 >>>>>> incompatible_features 0x0 >>>>>> compatible_features 0x0 >>>>>> autoclear_features 0x0 >>>>>> -refcount_order 4 >>>>>> +refcount_order (filtered) >>>>>> header_length 104 >>>>>> >>>>>> Header extension: >>>>> >>>>> Minor notes: >>>>> >>>>> 1. Maybe put the sed command into a function to avoid duplication? >>>> >>>> Hm, maybe, but that would increase the LoC, so I’m not sure whether it >>>> really would be a simplification. >>> >>> IMHO, in my opinion, regardless of LOC, duplicated code is almost always >>> bad. Common functions are mostly for the next guy that will be able to use >>> them instead of searching through code to see how this is done there and there. >> >> In my experience, people write tests without scanning common.filter for >> whether anyone has written the same code already. See the >> _filter_qemu_img_check example in this series. > Fair enough, but this can change. It won’t. It only changes when people use standardized functions that filter automatically. And writing that made me look into common.rc, and ah yes, we have a _check_test_img. So the correct thing to do is not to use _filter_qemu_img_check (or to expect people to scan the filter file for filters), but to use _check_test_img there. >>>>> 2. As I understand the test, it only checks the feature bits. >>>>> So maybe instead of blacklisting some of the values, white list >>>>> only the feature bits in the output? >>>> >>>> Hm. Seems reasonable, but then again I’d prefer a minimal change, and >>>> changing it to a whitelist would be more change... >>> >>> I don't think this is bad, again in my eyes, the code should be as friendly >>> as possible for the next guy that will have to change it, so adding >>> some more extra changes don't seem a problem to me. >> >> In my eyes tests aren’t code. > > And yet, writing tests is commonly known as a task > that developers don't really like to do, so making that even a tiniest bit > easier, is a good thing IMHO, My problem is precisely that I don’t see how such a suggestion makes it easier to write test code. The easiest way to write a test is to just dump ad-hoc code into a bash file. Sometimes that doesn’t work because what you want to test is more complex, so then you use Python. What I felt you were arguing about is changing existing test code, which is a completely different thing. In my experience, that happens much more rarely and is thus a thing we don’t need to optimize for. (And I’m saying this as the one who wrote this series which is just about changing existing test code.) Max
On Mon, 2019-09-30 at 16:04 +0200, Max Reitz wrote: > On 30.09.19 15:58, Maxim Levitsky wrote: > > On Mon, 2019-09-30 at 15:44 +0200, Max Reitz wrote: > > > On 30.09.19 15:40, Maxim Levitsky wrote: > > > > On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote: > > > > > On 29.09.19 18:31, Maxim Levitsky wrote: > > > > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > > > > > > > This test can run just fine with other values for refcount_bits, so we > > > > > > > should filter the value from qcow2.py's dump-header. > > > > > > > > > > > > > > (036 currently ignores user-specified image options, but that will be > > > > > > > fixed in the next patch.) > > > > > > > > > > > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > > > > > > --- > > > > > > > tests/qemu-iotests/036 | 9 ++++++--- > > > > > > > tests/qemu-iotests/036.out | 6 +++--- > > > > > > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 > > > > > > > index f06ff67408..69d0f9f903 100755 > > > > > > > --- a/tests/qemu-iotests/036 > > > > > > > +++ b/tests/qemu-iotests/036 > > > > > > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 > > > > > > > > > > > > > > # Without feature table > > > > > > > $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 > > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > > > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > > _img_info > > > > > > > > > > > > > > # With feature table containing bit 63 > > > > > > > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === > > > > > > > echo > > > > > > > _make_test_img 64M > > > > > > > $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 > > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > > > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > > > > > > > > > echo > > > > > > > echo === Repair image === > > > > > > > echo > > > > > > > _check_test_img -r all > > > > > > > > > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header > > > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \ > > > > > > > + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' > > > > > > > > > > > > > > # success, all done > > > > > > > echo "*** done" > > > > > > > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out > > > > > > > index e489b44386..998c2a8a35 100644 > > > > > > > --- a/tests/qemu-iotests/036.out > > > > > > > +++ b/tests/qemu-iotests/036.out > > > > > > > @@ -19,7 +19,7 @@ snapshot_offset 0x0 > > > > > > > incompatible_features 0x8000000000000000 > > > > > > > compatible_features 0x0 > > > > > > > autoclear_features 0x0 > > > > > > > -refcount_order 4 > > > > > > > +refcount_order (filtered) > > > > > > > header_length 104 > > > > > > > > > > > > > > qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 > > > > > > > @@ -53,7 +53,7 @@ snapshot_offset 0x0 > > > > > > > incompatible_features 0x0 > > > > > > > compatible_features 0x0 > > > > > > > autoclear_features 0x8000000000000000 > > > > > > > -refcount_order 4 > > > > > > > +refcount_order (filtered) > > > > > > > header_length 104 > > > > > > > > > > > > > > Header extension: > > > > > > > @@ -81,7 +81,7 @@ snapshot_offset 0x0 > > > > > > > incompatible_features 0x0 > > > > > > > compatible_features 0x0 > > > > > > > autoclear_features 0x0 > > > > > > > -refcount_order 4 > > > > > > > +refcount_order (filtered) > > > > > > > header_length 104 > > > > > > > > > > > > > > Header extension: > > > > > > > > > > > > Minor notes: > > > > > > > > > > > > 1. Maybe put the sed command into a function to avoid duplication? > > > > > > > > > > Hm, maybe, but that would increase the LoC, so I’m not sure whether it > > > > > really would be a simplification. > > > > > > > > IMHO, in my opinion, regardless of LOC, duplicated code is almost always > > > > bad. Common functions are mostly for the next guy that will be able to use > > > > them instead of searching through code to see how this is done there and there. > > > > > > In my experience, people write tests without scanning common.filter for > > > whether anyone has written the same code already. See the > > > _filter_qemu_img_check example in this series. > > > > Fair enough, but this can change. > > It won’t. > > It only changes when people use standardized functions that filter > automatically. And writing that made me look into common.rc, and ah > yes, we have a _check_test_img. So the correct thing to do is not to > use _filter_qemu_img_check (or to expect people to scan the filter file > for filters), but to use _check_test_img there. > > > > > > > 2. As I understand the test, it only checks the feature bits. > > > > > > So maybe instead of blacklisting some of the values, white list > > > > > > only the feature bits in the output? > > > > > > > > > > Hm. Seems reasonable, but then again I’d prefer a minimal change, and > > > > > changing it to a whitelist would be more change... > > > > > > > > I don't think this is bad, again in my eyes, the code should be as friendly > > > > as possible for the next guy that will have to change it, so adding > > > > some more extra changes don't seem a problem to me. > > > > > > In my eyes tests aren’t code. > > > > And yet, writing tests is commonly known as a task > > that developers don't really like to do, so making that even a tiniest bit > > easier, is a good thing IMHO, > > My problem is precisely that I don’t see how such a suggestion makes it > easier to write test code. > > The easiest way to write a test is to just dump ad-hoc code into a bash > file. Sometimes that doesn’t work because what you want to test is more > complex, so then you use Python. > > > What I felt you were arguing about is changing existing test code, which > is a completely different thing. In my experience, that happens much > more rarely and is thus a thing we don’t need to optimize for. (And I’m > saying this as the one who wrote this series which is just about > changing existing test code.) I won't argue on this! It all depends on lot and lot of things. Best regards, Maxim Levitsky
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 index f06ff67408..69d0f9f903 100755 --- a/tests/qemu-iotests/036 +++ b/tests/qemu-iotests/036 @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63 # Without feature table $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857 -$PYTHON qcow2.py "$TEST_IMG" dump-header +$PYTHON qcow2.py "$TEST_IMG" dump-header \ + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' _img_info # With feature table containing bit 63 @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit === echo _make_test_img 64M $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63 -$PYTHON qcow2.py "$TEST_IMG" dump-header +$PYTHON qcow2.py "$TEST_IMG" dump-header \ + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' echo echo === Repair image === echo _check_test_img -r all -$PYTHON qcow2.py "$TEST_IMG" dump-header +$PYTHON qcow2.py "$TEST_IMG" dump-header \ + | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/' # success, all done echo "*** done" diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out index e489b44386..998c2a8a35 100644 --- a/tests/qemu-iotests/036.out +++ b/tests/qemu-iotests/036.out @@ -19,7 +19,7 @@ snapshot_offset 0x0 incompatible_features 0x8000000000000000 compatible_features 0x0 autoclear_features 0x0 -refcount_order 4 +refcount_order (filtered) header_length 104 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000 @@ -53,7 +53,7 @@ snapshot_offset 0x0 incompatible_features 0x0 compatible_features 0x0 autoclear_features 0x8000000000000000 -refcount_order 4 +refcount_order (filtered) header_length 104 Header extension: @@ -81,7 +81,7 @@ snapshot_offset 0x0 incompatible_features 0x0 compatible_features 0x0 autoclear_features 0x0 -refcount_order 4 +refcount_order (filtered) header_length 104 Header extension:
This test can run just fine with other values for refcount_bits, so we should filter the value from qcow2.py's dump-header. (036 currently ignores user-specified image options, but that will be fixed in the next patch.) Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/036 | 9 ++++++--- tests/qemu-iotests/036.out | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-)