Re: [PULL 6/6] iotests: add backup-discard-source

[Kevin Wolf <kwolf@redhat.com>]

Am 29.04.2024 um 20:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
> [Add John]
> 
> On 29.04.24 17:18, Richard Henderson wrote:
> > On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:
> > > Add test for a new backup option: discard-source.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> > > Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> > > Message-Id: <20240313152822.626493-6-vsementsov@yandex-team.ru>
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > >   .../qemu-iotests/tests/backup-discard-source  | 152 ++++++++++++++++++
> > >   .../tests/backup-discard-source.out           |   5 +
> > >   2 files changed, 157 insertions(+)
> > >   create mode 100755 tests/qemu-iotests/tests/backup-discard-source
> > >   create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
> > 
> > This fails check-python-minreqs
> > 
> >    https://gitlab.com/qemu-project/qemu/-/jobs/6739551782
> > 
> > It appears to be a pylint issue.
> > 
> > 
> 
> tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
> ==tests.backup-discard-source:[52:61]
> ==tests.copy-before-write:[54:63]
>             'file': {
>                 'driver': iotests.imgfmt,
>                 'file': {
>                     'driver': 'file',
>                     'filename': source_img,
>                 }
>             },
>             'target': {
>                 'driver': iotests.imgfmt, (duplicate-code)
> 
> 
> 
> Hmm. I don't think, that something should be changed in code.
> splitting out part of this json object to a function? That's a test
> for QMP command, and it's good that we see the command as is in one
> place. I can swap some lines or rename variables to hack the linter,
> but I'd prefer not doing so:)
> 
> 
> For me that looks like a false-positive: yes it's a duplication, but
> it should better be duplication, than complicating raw json objects by
> reusing common parts.
> 
> 
> What to do? As described in 22305c2a081b8b6 "python: Reduce strictness
> of pylint's duplicate-code check", this check could be either be
> disabled completely, or we can increase min-similarity-lines config
> value.
> 
> I'd just disable it completely. Any thoughts?

I think it would make sense to treat tests differently from real
production code. In tests/, some duplication is bound to happen and
trying to unify things across test cases (which would mean moving to
iotests.py) often doesn't make sense. On the other hand, in python/ we
would probably want to unify duplicated code.

Kevin