Re: [PATCH 05/14] iotests.py: filter compression type out

[Vladimir Sementsov-Ogievskiy]

16.07.2021 14:15, Max Reitz wrote:
> On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
> > We want iotests pass with both the default zlib compression and with
> > IMGOPTS='compression_type=zstd'.
> >
> > Actually the only test that is interested in real compression type in
> > test output is 287 (test for qcow2 compression type) and it's in bash.
> > So for now we can safely filter out compression type in all qcow2
> > tests.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/206.out    | 10 +++---
> >   tests/qemu-iotests/242.out    | 10 +++---
> >   tests/qemu-iotests/255.out    |  8 ++---
> >   tests/qemu-iotests/274.out    | 68 +++++++++++++++++------------------
> >   tests/qemu-iotests/280.out    |  2 +-
> >   tests/qemu-iotests/iotests.py | 13 ++++++-
> >   6 files changed, 61 insertions(+), 50 deletions(-)
>
> Looks OK, though I wonder if it weren’t better to have a filter that only 
> prints some options and explicitly filters out everything else.

That means larger work and larger audit of what actually each test wants to see 
in the output..

 (Well, actually, I’d prefer not to have the “Formatting…” line in the 
reference output at all, because I don’t see the point, but I suppose that can 
be considered a different problem.)

Hmm. I like the idea of dropping this line, I don't remember any bug that this 
line helped to catch, but we have to update it every time we add some new 
option. I can make a separate patch in v2 to just filter it out everywhere.

> [...]
>
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 80f0cb4f42..6a8cc1bad7 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -224,9 +224,18 @@ def qemu_img_verbose(*args):
> >                            % (-exitcode, ' '.join(qemu_img_args + list(args))))
> >       return exitcode
> > +def filter_img_create(text: str) -> str:
> > +    return re.sub('(compression_type=)(zlib|zstd)', r'\1COMPRESSION_TYPE',
> > +                  text)
> > +
> >   def qemu_img_pipe(*args: str) -> str:
> >       '''Run qemu-img and return its output'''
> > -    return qemu_img_pipe_and_status(*args)[0]
> > +    output =  qemu_img_pipe_and_status(*args)[0]
>
> There’s a superfluous space after '='.
>
> > +
> > +    if args[0] == 'create':
> > +        return filter_img_create(output)
> > +
> > +    return output
>
> Wouldn’t it make more sense to have this filter be in 
> qemu_img_pipe_and_status()?

Hmm probably someone want to not filter information out, then 
qemu_img_pipe_and_status() will be a way to get unfiltered output..

But I tend to agree, as in 02 I do generic logic in qemu_img_pipe_and_status(), 
so until we have a good reason, it's better to keep all generic logic in one 
place. OK, will move it to qemu_img_pipe_and_status()

> >   def qemu_img_log(*args):
> >       result = qemu_img_pipe(*args)
> > @@ -479,6 +488,8 @@ def filter_img_info(output, filename):
> >                         'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX',
> >                         line)
> >           line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line)
> > +        line = re.sub('(compression type: )(zlib|zstd)', r'\1COMPRESSION_TYPE',
> > +                      line)
> >           lines.append(line)
> >       return '\n'.join(lines)


--
Best regards,
Vladimir