Hi Willian
I totally agree with your comments
I'll move those functions outside the class
Let me update and repost the patch
Thanks
On Fri, Mar 20, 2020 at 6:22 PM Willian Rampazzo <wramp...@redhat.com>
wrote:
> Hi Oksana,
>
> On Fri, Mar 20, 2020 at 12:15 PM Oksana Vohchana <ovosh...@redhat.com>
> wrote:
> >
> > Provides new functions related to the rdma migration test
> > Adds functions to check if service RDMA is enabled and gets
> > the ip address on the interface where it was configured
> >
> > Signed-off-by: Oksana Vohchana <ovosh...@redhat.com>
> > ---
> > tests/acceptance/migration.py | 30 ++++++++++++++++++++++++++++++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/tests/acceptance/migration.py
> b/tests/acceptance/migration.py
> > index e4c39b85a1..a783f3915b 100644
> > --- a/tests/acceptance/migration.py
> > +++ b/tests/acceptance/migration.py
> > @@ -11,12 +11,17 @@
> >
> >
> > import tempfile
> > +import json
> > from avocado_qemu import Test
> > from avocado import skipUnless
> >
> > from avocado.utils import network
> > from avocado.utils import wait
> > from avocado.utils.path import find_command
> > +from avocado.utils.network.interfaces import NetworkInterface
> > +from avocado.utils.network.hosts import LocalHost
> > +from avocado.utils import service
> > +from avocado.utils import process
> >
> >
> > class Migration(Test):
> > @@ -58,6 +63,31 @@ class Migration(Test):
> > self.cancel('Failed to find a free port')
> > return port
> >
> > + def _if_rdma_enable(self):
> > + rdma_stat = service.ServiceManager()
> > + rdma = rdma_stat.status('rdma')
> > + return rdma
>
> You can just `return rdma_stat.status('rdma')` here! Also, as you are
> not using any of the class attributes or methods, if you make this
> method static, you don't need to call it with `None` as you did on
> patch 3 of this series.
>
> > +
> > + def _get_interface_rdma(self):
> > + cmd = 'rdma link show -j'
> > + out = json.loads(process.getoutput(cmd))
> > + try:
> > + for i in out:
> > + if i['state'] == 'ACTIVE':
> > + return i['netdev']
> > + except KeyError:
> > + return None
>
> Same comment about making this method static.
>
> Actually, if you are not using any of the attributes or methods from
> the Migration class on these two methods, I think you can define them
> as functions, outside of the Class. Does it make sense?
>
> > +
> > + def _get_ip_rdma(self, interface):
> > + local = LocalHost()
> > + network_in = NetworkInterface(interface, local)
> > + try:
> > + ip = network_in._get_interface_details()
> > + if ip:
> > + return ip[0]['addr_info'][0]['local']
> > + except:
> > + self.cancel("Incorrect interface configuration or device
> name")
> > +
>
> If you change the logic a bit and raise an exception here, instead of
> doing a `self.cancel`, you can also make this method static, or move
> it outside of the class. The cancel can be handled in the test, with
> the exception raised here.
>
> >
> > def test_migration_with_tcp_localhost(self):
> > dest_uri = 'tcp:localhost:%u' % self._get_free_port()
> > --
> > 2.21.1
> >
> >
>
> Let me know if the comments do not make sense.
>
> Willian
>
>
