Re: [PATCH-for-5.0 v3] Acceptance test: Fix to EXEC migration

[Philippe Mathieu-Daudé]

Hi David,

On 3/25/20 4:15 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (address@hidden) wrote:
> > On 3/25/20 3:10 PM, Oksana Voshchana wrote:
> > > Hi Philippe
> > > Thanks for the review
> > > I have some comments
> > >
> > > On Wed, Mar 25, 2020 at 2:30 PM Philippe Mathieu-Daudé
> > > <address@hidden <mailto:address@hidden>> wrote:
> > >
> > >      Hi Oksana,
> > >
> > >      v2 was
> > >      https://www.mail-archive.com/address@hidden/msg682899.html, so
> > >      this is v3. Please increment the version in the patch subject.
> > >
> > >      You could also send a simple "ping" to the specific patch, instead of
> > >      resending it.
> > >
> > >      On 3/25/20 12:31 PM, Oksana Vohchana wrote:
> > >       > The exec migration test isn't run a whole test scenario.
> > >       > This patch fixes it
> > >       >
> > >       > Signed-off-by: Oksana Vohchana <address@hidden
> > >      <mailto:address@hidden>>
> > >
> > >      v1 of this patch has already received reviewers tags
> > >      (https://www.mail-archive.com/address@hidden/msg679629.html),
> > >      please collect them and keep them when you resend the same patch:
> > >
> > > I have reposted patch without this fix because this change isn't related
> > > to the series:
> > > https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06919.html
> > > Is it make sense to keep this fix as a separate patch?
> > >
> > >      Fixes: 2e768cb682bf
> > >      Reviewed-by: Philippe Mathieu-Daudé <address@hidden
> > >      <mailto:address@hidden>>
> > >      Tested-by: Wainer dos Santos Moschetta <address@hidden
> > >      <mailto:address@hidden>>
> > >
> > >       > ---
> > >       >   tests/acceptance/migration.py | 6 +++---
> > >       >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >       >
> > >       > diff --git a/tests/acceptance/migration.py
> > >      b/tests/acceptance/migration.py
> > >       > index a8367ca023..0365289cda 100644
> > >       > --- a/tests/acceptance/migration.py
> > >       > +++ b/tests/acceptance/migration.py
> > >       > @@ -70,8 +70,8 @@ class Migration(Test):
> > >       >
> > >       >       @skipUnless(find_command('nc', default=False), "'nc'
> > >      command not found")
> > >       >       def test_migration_with_exec(self):
> > >       > -        """
> > >       > -        The test works for both netcat-traditional and
> > >      netcat-openbsd packages
> > >       > -        """
> > >       > +        """The test works for both netcat-traditional and
> > >      netcat-openbsd packages."""
> > >
> > >      Btw why are you changing the comment style?
> > >
> > > I got failure in PEP257
> >
> > OK, next time please add comment in the patch description too.
> >
> > >       >           free_port = self._get_free_port()
> > >       >           dest_uri = 'exec:nc -l localhost %u' % free_port
> > >       > +        src_uri = 'exec:nc localhost %u' % free_port
> > >       > +        self.do_migrate(dest_uri, src_uri)
> > >       >
> > >
> > >      Alex, if there is no Python testing pullreq, can you take this patch
> > >      via
> > >      your testing tree?
> >
> > Cc'ing David since it is migration related.
>
> I tend to leave the tests/acceptance to someone other than the migration
> tree; so feel free to take them via testing or trivial given the size.

Acceptance tests are in the same tests/acceptance directory for no 
particular reason. We thought they would share some common code but this 
code is in the python/qemu/ directory.

Similarly to C Qtests stored in the tests/qtest, acceptance are in 
tests/acceptance. tests/qtest have various maintainers, when a 
maintainer has interest in a qtest (subsystem covered by the test), the 
test is added to the subsystem maintained section. I'd rather see the 
same with acceptance tests. For example with Oksana's test, I can review 
that it correctly uses the acceptance testing framework, and it doesn't 
break the rest of the tests, but I have no idea if it properly tests the 
migration features it aims to.

Back to this particular patch, if Eduardo/Cleber ack I can send a 
pullreq for rc2.

> Dave
>
> > >      Thanks,
> > >
> > >      Phil.
> > >
> > > Thanks
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK