Re: [PATCH 15/18] iotests: Make 137 work with data_file

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:57 +0200, Max Reitz wrote:
> On 30.09.19 15:46, Maxim Levitsky wrote:
> > On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote:
> > > On 29.09.19 18:38, Maxim Levitsky wrote:
> > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > > > When using an external data file, there are no refcounts for data
> > > > > clusters.  We thus have to adjust the corruption test in this patch to
> > > > > not be based around a data cluster allocation, but the L2 table
> > > > > allocation (L2 tables are still refcounted with external data files).
> > > > > 
> > > > > Doing so means this test works both with and without external data
> > > > > files.
> > > > > 
> > > > > Signed-off-by: Max Reitz <address@hidden>
> > > > > ---
> > > > >  tests/qemu-iotests/137     | 10 ++++++----
> > > > >  tests/qemu-iotests/137.out |  4 +---
> > > > >  2 files changed, 7 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > > > > index 6cf2997577..dd3484205e 100755
> > > > > --- a/tests/qemu-iotests/137
> > > > > +++ b/tests/qemu-iotests/137
> > > > > @@ -138,14 +138,16 @@ $QEMU_IO \
> > > > >      "$TEST_IMG" 2>&1 | _filter_qemu_io
> > > > >  
> > > > >  # The dirty bit must not be set
> > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> > > > > +# (Filter the external data file bit)
> > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep 
> > > > > incompatible_features \
> > > > > +    | sed -e 's/0x4/0x0/'
> > > > 
> > > > Maybe it is better to filter all the feature bits, but the dirty bit,
> > > > since only it is needed here, so that when we start running tests with
> > > > more features, we won't need to do this again?
> > > 
> > > I’d hate a filter s/[02468ace]$/no dirty bit/ though.
> > 
> > Nothing a helper function can't solve IMHO, I would convert this to a 
> > number,
> > and then check bitwise the bit 2, assuming that is the dirty bit)
> > Again, note that my approach to code is to make it as easy as possible for 
> > the
> > next guy to change, so I am noticing such places. Eventually someone of us,
> > will be that next guy. Then again, I don't mind leaving this as is, just 
> > noting this.
> 
> Again, my approach to tests is that they aren’t classical code.
> 
> This is a very personal opinion, but I have found that tests that have
> the most ad-hoc code with the least function calls are the easiest to
> work with.
> 
> Tests that have a whole lot of infrastructure and try to have nice code
> are a horror to work with because you first have to understand how they
> work.

I guess everything should be in balance, but I understand very well
what you mean.

> 
> Tests should be simple, not complex.  Some ad-hoc filters do not make
> them complex as long as it’s obvious what they do.

Yea, but the point is that it makes it harder to test new features,
that slightly change the output of the tests, and break various tests that 
hardcode unneeded things.

Pretty much exactly what this patch series does, 
is to remove some of the ad-hoc stuff to make the
tests work with a new feature. 

A bit more generic tests, might be able
to reduce this work. Anyway I won't argue with this as well, its all matter of
balance, both extremes are no doubt bad.

> 
> 
> Also, the correct approach here is not to do number crunching in bash.
> It is to change qcow2.py to emit more easily filterable information.
100% agree. 

> 
> Max
> 

Best regards,
        Maxim Levitsky