Also add flake8 target in tests Makefile and rewrite
some parts to be more pythonic.

Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
---
 HACKING.md           |  2 +-
 test/Makefile.am     | 15 ++++++++++++---
 test/conftest.py     | 15 +++++++++++++++
 test/libvirttest.py  | 51 ++++++++++++++++++++++++++++-----------------------
 test/test_connect.py | 35 +++++++++++++++--------------------
 test/test_domain.py  | 39 +++++++++++++++++----------------------
 test/travis-run      |  4 ++--
 7 files changed, 90 insertions(+), 71 deletions(-)
 create mode 100644 test/conftest.py

diff --git a/HACKING.md b/HACKING.md
index 75aa6d0..30d321c 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -35,7 +35,7 @@ Running from git repository
     make check
     ```
 
-    The test tool requires python3 and python3-dbus.
+    The test tool requires python3, pytest and python3-dbus.
 
 
   * To run libvirt-dbus directly from the build dir without installing it
diff --git a/test/Makefile.am b/test/Makefile.am
index d3997f3..554c433 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -1,17 +1,26 @@
 test_helpers = \
-	libvirttest.py
+	libvirttest.py \
+	conftest.py
 
 test_programs = \
 	test_connect.py \
 	test_domain.py
 
-TESTS = $(test_programs)
-
 EXTRA_DIST = \
 	$(test_helpers) \
 	$(test_programs) \
 	travis-run
 
+TEST_PATH=./
+
+lint:
+	flake8 --exclude=.tox --ignore=E501
+
+test:
+	py.test --verbose --color=yes $(TEST_PATH)
+
+check: lint test
+
 TESTS_ENVIRONMENT = \
 	abs_top_builddir=$(abs_top_builddir) \
 	VIRT_DBUS_INTERFACES_DIR=$(abs_top_srcdir)/data
diff --git a/test/conftest.py b/test/conftest.py
new file mode 100644
index 0000000..a468599
--- /dev/null
+++ b/test/conftest.py
@@ -0,0 +1,15 @@
+import os
+import subprocess
+import pytest
+
+
+@pytest.fixture(scope="session", autouse=True)
+def dbus_daemon_session():
+    """Fixture to start D-Bus message bus session daemon for use in the whole test suite.
+    """
+    dbus_daemon = subprocess.Popen(['dbus-daemon', '--session', '--print-address'],
+                                   stdout=subprocess.PIPE, universal_newlines=True)
+    os.environ['DBUS_SESSION_BUS_ADDRESS'] = dbus_daemon.stdout.readline().strip()
+    yield
+    dbus_daemon.terminate()
+    dbus_daemon.wait(timeout=10)
diff --git a/test/libvirttest.py b/test/libvirttest.py
index 6a00aea..fc8c5c4 100644
--- a/test/libvirttest.py
+++ b/test/libvirttest.py
@@ -1,34 +1,33 @@
 #!/usr/bin/python3
 
-from dbus.mainloop.glib import DBusGMainLoop
-from gi.repository import GLib
-import dbus
 import os
 import subprocess
 import time
-import unittest
+import pytest
+from gi.repository import GLib
+from dbus.mainloop.glib import DBusGMainLoop
+import dbus
 
-root = os.environ.get('abs_top_builddir', os.path.dirname(os.path.dirname(__file__)))
-exe = os.path.join(root, 'src', 'libvirt-dbus')
+ROOT = os.environ.get('abs_top_builddir', os.path.dirname(os.path.dirname(__file__)))
+EXE = os.path.join(ROOT, 'src', 'libvirt-dbus')
 
 DBusGMainLoop(set_as_default=True)
 
-class TestCase(unittest.TestCase):
-    @classmethod
-    def setUpClass(cls):
-        cls.bus = subprocess.Popen(['dbus-daemon', '--session', '--print-address'],
-            stdout=subprocess.PIPE, universal_newlines=True)
-        os.environ['DBUS_SESSION_BUS_ADDRESS'] = cls.bus.stdout.readline().strip()
 
-    @classmethod
-    def tearDownClass(cls):
-        cls.bus.terminate()
-        cls.bus.wait(timeout=10)
+class BaseTestClass():
+    """ Base test class for whole test suite
+    """
+    connect = None
+    bus = None
+    libvirt_dbus = None
+    loop = False
 
-    def setUp(self):
+    @pytest.fixture(autouse=True)
+    def libvirt_dbus_setup(self, request):
+        """Start libvirt-dbus for each test function
+        """
         os.environ['LIBVIRT_DEBUG'] = '3'
-
-        self.daemon = subprocess.Popen([exe])
+        self.libvirt_dbus = subprocess.Popen([EXE])
         self.bus = dbus.SessionBus()
 
         for i in range(10):
@@ -41,12 +40,18 @@ class TestCase(unittest.TestCase):
         obj = self.bus.get_object('org.libvirt', '/org/libvirt/Test')
         self.connect = dbus.Interface(obj, 'org.libvirt.Connect')
 
-    def tearDown(self):
-        self.daemon.terminate()
-        self.daemon.wait(timeout=10)
+    @pytest.fixture(autouse=True)
+    def libvirt_dbus_teardown(self):
+        """Terminate libvirt-dbus at the teardown of each test
+        """
+        yield
+        self.libvirt_dbus.terminate()
+        self.libvirt_dbus.wait(timeout=10)
 
     def main_loop(self):
-        self.assertFalse(getattr(self, 'loop', False))
+        """Initializes the mainloop
+        """
+        assert getattr(self, 'loop', False) is False
 
         def timeout():
             self.loop.quit()
diff --git a/test/test_connect.py b/test/test_connect.py
index 4ec3fe0..14f70d5 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -1,10 +1,9 @@
-#!/usr/bin/python3
-
 import dbus
 import libvirttest
-import unittest
 
-minimal_xml = '''
+
+class TestConnect(libvirttest.BaseTestClass):
+    minimal_xml = '''
     <domain type="test">
       <name>foo</name>
       <memory>1024</memory>
@@ -12,16 +11,15 @@ minimal_xml = '''
         <type>hvm</type>
       </os>
     </domain>
-'''
+    '''
 
-class TestConnect(libvirttest.TestCase):
     def test_list_domains(self):
         domains = self.connect.ListDomains(0)
-        self.assertEqual(type(domains), dbus.Array)
-        self.assertEqual(len(domains), 1)
+        assert isinstance(domains, dbus.Array)
+        assert len(domains) == 1
 
         for path in domains:
-            self.assertEqual(type(path), dbus.ObjectPath)
+            assert isinstance(path, dbus.ObjectPath)
             domain = self.bus.get_object('org.libvirt', path)
 
             # ensure the path exists by calling Introspect on it
@@ -29,29 +27,26 @@ class TestConnect(libvirttest.TestCase):
 
     def test_create(self):
         def domain_started(name, path):
-            self.assertEqual(name, 'foo')
-            self.assertEqual(type(path), dbus.ObjectPath)
+            assert name == 'foo'
+            assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
         self.connect.connect_to_signal('DomainStarted', domain_started)
 
-        path = self.connect.CreateXML(minimal_xml, 0)
-        self.assertEqual(type(path), dbus.ObjectPath)
+        path = self.connect.CreateXML(self.minimal_xml, 0)
+        assert isinstance(path, dbus.ObjectPath)
 
         self.main_loop()
 
     def test_define(self):
         def domain_defined(name, path):
-            self.assertEqual(name, 'foo')
-            self.assertEqual(type(path), dbus.ObjectPath)
+            assert name == 'foo'
+            assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
         self.connect.connect_to_signal('DomainDefined', domain_defined)
 
-        path = self.connect.DefineXML(minimal_xml)
-        self.assertEqual(type(path), dbus.ObjectPath)
+        path = self.connect.DefineXML(self.minimal_xml)
+        assert isinstance(path, dbus.ObjectPath)
 
         self.main_loop()
-
-if __name__ == '__main__':
-    unittest.main(verbosity=2)
diff --git a/test/test_domain.py b/test/test_domain.py
index b1ab7a5..b176c29 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -1,10 +1,8 @@
-#!/usr/bin/python3
-
 import dbus
 import libvirttest
-import unittest
 
-class TestDomain(libvirttest.TestCase):
+
+class TestDomain(libvirttest.BaseTestClass):
     def domain(self):
         path = self.connect.ListDomains(0)[0]
         obj = self.bus.get_object('org.libvirt', path)
@@ -14,21 +12,21 @@ class TestDomain(libvirttest.TestCase):
         obj, domain = self.domain()
 
         props = obj.GetAll('org.libvirt.Domain', dbus_interface=dbus.PROPERTIES_IFACE)
-        self.assertEqual(type(props['Name']), dbus.String)
-        self.assertEqual(type(props['UUID']), dbus.String)
-        self.assertEqual(type(props['Id']), dbus.UInt32)
-        self.assertEqual(type(props['Vcpus']), dbus.UInt32)
-        self.assertEqual(type(props['OSType']), dbus.String)
-        self.assertEqual(type(props['Active']), dbus.Boolean)
-        self.assertEqual(type(props['Persistent']), dbus.Boolean)
-        self.assertEqual(type(props['State']), dbus.String)
-        self.assertEqual(type(props['Autostart']), dbus.Boolean)
+        assert isinstance(props['Name'], dbus.String)
+        assert isinstance(props['UUID'], dbus.String)
+        assert isinstance(props['Id'], dbus.UInt32)
+        assert isinstance(props['Vcpus'], dbus.UInt32)
+        assert isinstance(props['OSType'], dbus.String)
+        assert isinstance(props['Active'], dbus.Boolean)
+        assert isinstance(props['Persistent'], dbus.Boolean)
+        assert isinstance(props['State'], dbus.String)
+        assert isinstance(props['Autostart'], dbus.Boolean)
 
         # Call all methods except Reset and GetStats, because the test backend
         # doesn't support those
 
         xml = domain.GetXMLDesc(0)
-        self.assertEqual(type(xml), dbus.String)
+        assert isinstance(xml, dbus.String)
 
         domain.Reboot(0)
         domain.Shutdown()
@@ -38,8 +36,8 @@ class TestDomain(libvirttest.TestCase):
 
     def test_shutdown(self):
         def domain_stopped(name, path):
-            self.assertEqual(name, 'test')
-            self.assertEqual(type(path), dbus.ObjectPath)
+            assert name == 'test'
+            assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
         self.connect.connect_to_signal('DomainStopped', domain_stopped)
@@ -48,14 +46,14 @@ class TestDomain(libvirttest.TestCase):
         domain.Shutdown()
 
         state = obj.Get('org.libvirt.Domain', 'State', dbus_interface=dbus.PROPERTIES_IFACE)
-        self.assertEqual(state, 'shutoff')
+        assert state == 'shutoff'
 
         self.main_loop()
 
     def test_undefine(self):
         def domain_undefined(name, path):
-            self.assertEqual(name, 'test')
-            self.assertEqual(type(path), dbus.ObjectPath)
+            assert name == 'test'
+            assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
         self.connect.connect_to_signal('DomainUndefined', domain_undefined)
@@ -65,6 +63,3 @@ class TestDomain(libvirttest.TestCase):
         domain.Undefine()
 
         self.main_loop()
-
-if __name__ == '__main__':
-    unittest.main(verbosity=2)
diff --git a/test/travis-run b/test/travis-run
index 7577253..c2ab729 100755
--- a/test/travis-run
+++ b/test/travis-run
@@ -22,7 +22,7 @@ sudo chroot "$CHROOT" << EOF
 set -ex
 # install build deps
 apt-get update
-apt-get install -y dh-autoreconf pkg-config libvirt-dev libglib2.0-dev libtool python3-gi python3-dbus dbus
+apt-get install -y dh-autoreconf pkg-config libvirt-dev libglib2.0-dev libtool python3-gi python3-dbus dbus pytest
 
 # run build and tests as user
 chown -R buildd:buildd /build
@@ -31,6 +31,6 @@ set -ex
 cd /build/src
 ./autogen.sh
 make -j4
-make check || { cat test-suite.log; exit 1; }
+make check
 EOU
 EOF
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Mar 22, 2018 at 01:06:56PM +0100, Katerina Koukiou wrote:
>Also add flake8 target in tests Makefile and rewrite
>some parts to be more pythonic.
>

Using conjunctions like 'also' and 'and' this way in the commit message
is usually a sign that the commit contains multiple logical changes
and can be split into multiple commits.

https://www.berrange.com/tags/commit-message/

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Mar 22, 2018 at 01:06:56PM +0100, Katerina Koukiou wrote:
> Also add flake8 target in tests Makefile and rewrite
> some parts to be more pythonic.
> 
> Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> ---
>  HACKING.md           |  2 +-
>  test/Makefile.am     | 15 ++++++++++++---
>  test/conftest.py     | 15 +++++++++++++++
>  test/libvirttest.py  | 51 ++++++++++++++++++++++++++++-----------------------
>  test/test_connect.py | 35 +++++++++++++++--------------------
>  test/test_domain.py  | 39 +++++++++++++++++----------------------
>  test/travis-run      |  4 ++--
>  7 files changed, 90 insertions(+), 71 deletions(-)
>  create mode 100644 test/conftest.py
> 
> diff --git a/HACKING.md b/HACKING.md
> index 75aa6d0..30d321c 100644
> --- a/HACKING.md
> +++ b/HACKING.md
> @@ -35,7 +35,7 @@ Running from git repository
>      make check
>      ```
>  
> -    The test tool requires python3 and python3-dbus.
> +    The test tool requires python3, pytest and python3-dbus.

We should explicitly say that we need python3-pytest.

>    * To run libvirt-dbus directly from the build dir without installing it
> diff --git a/test/Makefile.am b/test/Makefile.am
> index d3997f3..554c433 100644
> --- a/test/Makefile.am
> +++ b/test/Makefile.am
> @@ -1,17 +1,26 @@
>  test_helpers = \
> -	libvirttest.py
> +	libvirttest.py \
> +	conftest.py
>  
>  test_programs = \
>  	test_connect.py \
>  	test_domain.py
>  
> -TESTS = $(test_programs)
> -
>  EXTRA_DIST = \
>  	$(test_helpers) \
>  	$(test_programs) \
>  	travis-run
>  
> +TEST_PATH=./

Just a hint, this is never a good idea, it's always better to use
absolute path if possible, in this case it could be

TEST_PATH = $(abs_top_srcdir)/test

> +
> +lint:
> +	flake8 --exclude=.tox --ignore=E501

I'm not sure, whether we need to run flake8 on the test files, I would
leave it out for now, we can add it later.

> +
> +test:
> +	py.test --verbose --color=yes $(TEST_PATH)

This would not work as expected, the py.test is the same as py.test-3.

> +
> +check: lint test
> +

We don't need to change the makefile at all since we can make the
test_connect.py and test_domain.py as a standalone executable, see
my notes below.

>  TESTS_ENVIRONMENT = \
>  	abs_top_builddir=$(abs_top_builddir) \
>  	VIRT_DBUS_INTERFACES_DIR=$(abs_top_srcdir)/data
> diff --git a/test/conftest.py b/test/conftest.py
> new file mode 100644
> index 0000000..a468599
> --- /dev/null
> +++ b/test/conftest.py
> @@ -0,0 +1,15 @@
> +import os
> +import subprocess
> +import pytest

Nitpick: alphabetic order is nicer :).

Anyway, the test changes and the rewrite to pytest is OK.  I'm attaching
a patch with changes that would make the tests as standalone executable.
In this case the patch is better then describing it.

If you agree with the changes you can send a v2.

With these changes it should be possible to run "make check" to run
the whole test suite or "./run test/test_connect.py [pytest args]"
to run the single test with pytest arguments.

Thanks for the patch!

Pavel
diff --git a/HACKING.md b/HACKING.md
index 30d321c..bb22fd6 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -35,7 +35,7 @@ Running from git repository
     make check
     ```
 
-    The test tool requires python3, pytest and python3-dbus.
+    The test tool requires python3, python3-pytest and python3-dbus.
 
 
   * To run libvirt-dbus directly from the build dir without installing it
diff --git a/test/Makefile.am b/test/Makefile.am
index 554c433..acb2d33 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -11,15 +11,7 @@ EXTRA_DIST = \
 	$(test_programs) \
 	travis-run
 
-TEST_PATH=./
-
-lint:
-	flake8 --exclude=.tox --ignore=E501
-
-test:
-	py.test --verbose --color=yes $(TEST_PATH)
-
-check: lint test
+TESTS = $(test_programs)
 
 TESTS_ENVIRONMENT = \
 	abs_top_builddir=$(abs_top_builddir) \
diff --git a/test/conftest.py b/test/conftest.py
index a468599..3bce7b7 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -1,6 +1,6 @@
 import os
-import subprocess
 import pytest
+import subprocess
 
 
 @pytest.fixture(scope="session", autouse=True)
diff --git a/test/libvirttest.py b/test/libvirttest.py
index fc8c5c4..a7e242e 100644
--- a/test/libvirttest.py
+++ b/test/libvirttest.py
@@ -1,12 +1,12 @@
-#!/usr/bin/python3
-
+from dbus.mainloop.glib import DBusGMainLoop
+from gi.repository import GLib
+import dbus
 import os
+import pytest
 import subprocess
+import sys
 import time
-import pytest
-from gi.repository import GLib
-from dbus.mainloop.glib import DBusGMainLoop
-import dbus
+
 
 ROOT = os.environ.get('abs_top_builddir', os.path.dirname(os.path.dirname(__file__)))
 EXE = os.path.join(ROOT, 'src', 'libvirt-dbus')
@@ -14,6 +14,10 @@ EXE = os.path.join(ROOT, 'src', 'libvirt-dbus')
 DBusGMainLoop(set_as_default=True)
 
 
+def run():
+    exit(pytest.main(sys.argv))
+
+
 class BaseTestClass():
     """ Base test class for whole test suite
     """
diff --git a/test/test_connect.py b/test/test_connect.py
index 14f70d5..a52140c 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -1,3 +1,5 @@
+#!/usr/bin/python3
+
 import dbus
 import libvirttest
 
@@ -50,3 +52,7 @@ class TestConnect(libvirttest.BaseTestClass):
         assert isinstance(path, dbus.ObjectPath)
 
         self.main_loop()
+
+
+if __name__ == '__main__':
+    libvirttest.run()
diff --git a/test/test_domain.py b/test/test_domain.py
index b176c29..1bf9c1b 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -1,3 +1,5 @@
+#!/usr/bin/python3
+
 import dbus
 import libvirttest
 
@@ -63,3 +65,7 @@ class TestDomain(libvirttest.BaseTestClass):
         domain.Undefine()
 
         self.main_loop()
+
+
+if __name__ == '__main__':
+    libvirttest.run()
diff --git a/test/travis-run b/test/travis-run
index c2ab729..80b6dec 100755
--- a/test/travis-run
+++ b/test/travis-run
@@ -22,7 +22,7 @@ sudo chroot "$CHROOT" << EOF
 set -ex
 # install build deps
 apt-get update
-apt-get install -y dh-autoreconf pkg-config libvirt-dev libglib2.0-dev libtool python3-gi python3-dbus dbus pytest
+apt-get install -y dh-autoreconf pkg-config libvirt-dev libglib2.0-dev libtool python3-gi python3-dbus python3-pytest dbus
 
 # run build and tests as user
 chown -R buildd:buildd /build
@@ -31,6 +31,6 @@ set -ex
 cd /build/src
 ./autogen.sh
 make -j4
-make check
+make check || { cat test-suite.log; exit 1; }
 EOU
 EOF
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list