Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Maxim Levitsky]

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 <address@hidden>
> > > > > > > ---
> > > > > > >  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