[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH-for-5.0 v3] Acceptance test: Fix to EXEC migration
Date: Mon, 30 Mar 2020 13:29:37 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

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

     v1 of this patch has already received reviewers tags
     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:
Is it make sense to keep this fix as a separate patch?

     Fixes: 2e768cb682bf
     Reviewed-by: Philippe Mathieu-Daudé <address@hidden
     Tested-by: Wainer dos Santos Moschetta <address@hidden

      > ---
      >   tests/acceptance/migration.py | 6 +++---
      >   1 file changed, 3 insertions(+), 3 deletions(-)
      > diff --git a/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
     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.





Dr. David Alan Gilbert / address@hidden / Manchester, UK

reply via email to

[Prev in Thread] Current Thread [Next in Thread]