[PATCH] Acceptance test: Fix to EXEC migration

Oksana Vohchana posted 1 patch 1 week ago
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200325113138.20337-1-ovoshcha@redhat.com
tests/acceptance/migration.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

[PATCH] Acceptance test: Fix to EXEC migration

Posted by Oksana Vohchana 1 week ago
The exec migration test isn't run a whole test scenario.
This patch fixes it

Signed-off-by: Oksana Vohchana <ovoshcha@redhat.com>
---
 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."""
         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)
-- 
2.21.1


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

Posted by Philippe Mathieu-Daudé 1 week ago
Hi Oksana,

v2 was 
https://www.mail-archive.com/qemu-devel@nongnu.org/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 <ovoshcha@redhat.com>

v1 of this patch has already received reviewers tags 
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg679629.html), 
please collect them and keep them when you resend the same patch:

Fixes: 2e768cb682bf
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

> ---
>   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?

>           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?

Thanks,

Phil.


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

Posted by Oksana Voshchana 1 week ago
Hi Philippe
Thanks for the review
I have some comments

On Wed, Mar 25, 2020 at 2:30 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Hi Oksana,
>
> v2 was
> https://www.mail-archive.com/qemu-devel@nongnu.org/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 <ovoshcha@redhat.com>
>
> v1 of this patch has already received reviewers tags
> (https://www.mail-archive.com/qemu-devel@nongnu.org/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é <philmd@redhat.com>
> Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>
> > ---
> >   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


> >           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?
>
> Thanks,
>
> Phil.
>
> Thanks

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

Posted by Philippe Mathieu-Daudé 1 week ago
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é 
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     Hi Oksana,
> 
>     v2 was
>     https://www.mail-archive.com/qemu-devel@nongnu.org/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 <ovoshcha@redhat.com
>     <mailto:ovoshcha@redhat.com>>
> 
>     v1 of this patch has already received reviewers tags
>     (https://www.mail-archive.com/qemu-devel@nongnu.org/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?

As we are in freeze and this is a fix, it is fine to reply to your own 
patch with

"Ping? As this is a fix, can we get this single patch merged for 5.0 
please? Thanks!"

You are responsible of tracking your own patches and ping them (every 
week) if they are ignored.

> 
>     Fixes: 2e768cb682bf
>     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com
>     <mailto:wainersm@redhat.com>>
> 
>      > ---
>      >   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
> 
> 
>      >           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?
> 
>     Thanks,
> 
>     Phil.
> 
> Thanks


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

Posted by Philippe Mathieu-Daudé 1 week ago
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é 
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     Hi Oksana,
> 
>     v2 was
>     https://www.mail-archive.com/qemu-devel@nongnu.org/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 <ovoshcha@redhat.com
>     <mailto:ovoshcha@redhat.com>>
> 
>     v1 of this patch has already received reviewers tags
>     (https://www.mail-archive.com/qemu-devel@nongnu.org/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é <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com
>     <mailto:wainersm@redhat.com>>
> 
>      > ---
>      >   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.

> 
>     Thanks,
> 
>     Phil.
> 
> Thanks


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

Posted by Dr. David Alan Gilbert 1 week ago
* Philippe Mathieu-Daudé (philmd@redhat.com) 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é
> > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> > 
> >     Hi Oksana,
> > 
> >     v2 was
> >     https://www.mail-archive.com/qemu-devel@nongnu.org/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 <ovoshcha@redhat.com
> >     <mailto:ovoshcha@redhat.com>>
> > 
> >     v1 of this patch has already received reviewers tags
> >     (https://www.mail-archive.com/qemu-devel@nongnu.org/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é <philmd@redhat.com
> >     <mailto:philmd@redhat.com>>
> >     Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com
> >     <mailto:wainersm@redhat.com>>
> > 
> >      > ---
> >      >   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.

Dave

> > 
> >     Thanks,
> > 
> >     Phil.
> > 
> > Thanks
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


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

Posted by Philippe Mathieu-Daudé 1 week ago
Hi David,

On 3/25/20 4:15 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) 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é
>>> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>>>
>>>      Hi Oksana,
>>>
>>>      v2 was
>>>      https://www.mail-archive.com/qemu-devel@nongnu.org/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 <ovoshcha@redhat.com
>>>      <mailto:ovoshcha@redhat.com>>
>>>
>>>      v1 of this patch has already received reviewers tags
>>>      (https://www.mail-archive.com/qemu-devel@nongnu.org/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é <philmd@redhat.com
>>>      <mailto:philmd@redhat.com>>
>>>      Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com
>>>      <mailto:wainersm@redhat.com>>
>>>
>>>       > ---
>>>       >   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 / dgilbert@redhat.com / Manchester, UK
>