Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Eric Blake]

On 8/20/20 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> # MYPYPATH=../../python/ mypy 300
> 300:154: error: Item "None" of "Optional[Match[Any]]" has no attribute 
> "group"
> Found 1 error in 1 file (checked 1 source file)
>
> - the only complain. Suggest a fix:
>
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> index c6d86b1dbc..0241903743 100755
> --- a/tests/qemu-iotests/300
> +++ b/tests/qemu-iotests/300
> @@ -148,11 +148,11 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>               result = vm.qmp('human-monitor-command',
>                               command_line='info migrate_parameters')
>
> -            hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n  
> .*)*\n',
> -                                    result['return'], flags=re.MULTILINE)
> +            m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
> +                          result['return'], flags=re.MULTILINE)
> +            hmp_mapping = m.group(0).replace('\r', '') if m else None
>
> -            self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
> -                             self.to_hmp_mapping(mapping))
> +            self.assertEqual(hmp_mapping, self.to_hmp_mapping(mapping))
>           else:
>               self.assert_qmp(result, 'error/desc', error)

Thanks; I'll fold that in to v5.

> ===
>
> # flake8 300
> 300:24:1: F401 'time' imported but unused
> 300:34:1: E302 expected 2 blank lines, found 1
> 300:42:67: E502 the backslash is redundant between brackets
> 300:47:67: E502 the backslash is redundant between brackets
> 300:67:53: E502 the backslash is redundant between brackets
> 300:122:9: E125 continuation line with same indent as next logical line
> 300:134:9: E125 continuation line with same indent as next logical line
> 300:185:52: E502 the backslash is redundant between brackets
> 300:186:72: E502 the backslash is redundant between brackets
> 300:285:77: E502 the backslash is redundant between brackets
> 300:305:77: E502 the backslash is redundant between brackets
> 300:306:78: E502 the backslash is redundant between brackets
> 300:330:77: E502 the backslash is redundant between brackets
> 300:350:77: E502 the backslash is redundant between brackets
> 300:385:57: E502 the backslash is redundant between brackets
> 300:386:59: E502 the backslash is redundant between brackets
> 300:387:67: E502 the backslash is redundant between brackets
> 300:412:78: E502 the backslash is redundant between brackets
> 300:425:78: E502 the backslash is redundant between brackets
> 300:435:78: E502 the backslash is redundant between brackets
> 300:436:76: E502 the backslash is redundant between brackets
> 300:451:66: E502 the backslash is redundant between brackets
> 300:473:78: E502 the backslash is redundant between brackets
> 300:474:79: E502 the backslash is redundant between brackets
> 300:488:78: E502 the backslash is redundant between brackets
> 300:489:77: E502 the backslash is redundant between brackets
>
> - I post it just because ALE plugin in vim highlights all these things 
> for me. Up to you, I don't ask you to fix it.

Seems like an easy enough touchup to make.


> > +    def test_alias_on_both_migration(self) -> None:
> > +        src_map = self.mapping(self.src_node_name, 'node-alias',
> > +                               self.src_bmap_name, 'bmap-alias')
> > +
> > +        dst_map = self.mapping(self.dst_node_name, 'node-alias',
> > +                               self.dst_bmap_name, 'bmap-alias')
> > +
> > +        self.set_mapping(self.vm_a, src_map)
> > +        self.set_mapping(self.vm_b, dst_map)
> > +        self.migrate()
> > +        self.verify_dest_error(None)
>
> Hmm, probably verify_dest_error() should be called from migrate(), as 
> you call it (almost) always after migrate()

This one I did not touch; it can be a followup patch if desired.

> > +
> > +
>
> [..]
>
> > +    def test_unused_mapping_on_dst(self) -> None:
> > +        # Let the source not send any bitmaps
> > +        self.set_mapping(self.vm_a, [])
> > +
> > +        # Establish some mapping on the destination
> > +        self.set_mapping(self.vm_b, [])
>
> Seems, you wanted to specify non-empty mapping for vm_b, yes?
> With any non-empty mapping here, just to better correspond to the comments:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> (or keep this case with both mappings empty, and add one similar with 
> empty mapping only on src)

Adding another case will now have to be a separate patch.  At any rate, 
I've taken the liberty of including your R-b in my staging tree for the 
pending pull request, let me know if I should drop it.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org