Merge lp:~mvo/click/coverage into lp:click/devel

[Michael Vogt]

This branch runs the tests with the python{,3}-coverage framework (if its installed). This gives us details about the test code coverage and can also be used to report to the QA dashboard. The coverage data is generated during the build so that jenkins can pick it up.

Feedback welcome, especially if the gimock.py changes can be done in a different/more elegant way :)

[lp:~mvo/click/coverage]

458. By Michael Vogt on 2014-06-09

fix incorrect shuffle of sys import

[Colin Watson (cjwatson)]

review: Needs Fixing

[lp:~mvo/click/coverage]

459. By Michael Vogt on 2014-06-09

use python3-coverage instead of python-coverage (thanks to Colin!)

460. By Michael Vogt on 2014-06-09

.coveragerc: exclude all of the tests code

[Barry Warsaw (barry)]

On Jun 09, 2014, at 01:41 PM, Colin Watson wrote:

>It looks like this will always re-exec with python-coverage, even if the test
>suite was run with bare python. This seems a bit inconsistent. It would be
>nice to only switch to the coverage executable if we were running under
>coverage to start with. How about 'if "coverage" in sys.modules' as a way to
>test for that?

I'm working on getting coverage for system-image, including the dbus
subprocesses. There's this page:

http://nedbatchelder.com/code/coverage/subprocess.html

I think it makes sense to set the envar COVERAGE_PROCESS_START and check for
that to know whether you're running under coverage or not.

>I know os._exit(0) caused problems for coverage, but I'm a bit concerned
>about accidentally returning into test code and causing havoc. How about
>sys.exit(0) instead, which should still generate coverage data but will
>ensure we don't double-run test code?

sys.exit() raises a SystemExit exception, but os._exit() calls exit(2), so the
latter doesn't give Python a chance to intervene.

[Colin Watson (cjwatson)]

On Mon, Jun 09, 2014 at 02:06:32PM -0000, Barry Warsaw wrote:
> I'm working on getting coverage for system-image, including the dbus
> subprocesses. There's this page:
>
> http://nedbatchelder.com/code/coverage/subprocess.html
>
> I think it makes sense to set the envar COVERAGE_PROCESS_START and check for
> that to know whether you're running under coverage or not.

Right, this is fine as long as we ensure that that's set consistently.
I didn't want to recommend anything overly complicated without knowing
how QA is going to be running this.

> sys.exit() raises a SystemExit exception, but os._exit() calls
> exit(2), so the latter doesn't give Python a chance to intervene.

Indeed. But I was using os._exit exactly so that Python wouldn't run
cleanups in both the parent and child processes (similarly, I generally
use _exit from forked child processes in C code). In this case coverage
is unusual in that we actively want to run its cleanup code in both
processes, so we have a choice of either making an exception
specifically for coverage in some way (seems tricky as we don't have the
coverage object in hand in gimock) or of taking special care with
everything else that might end up running twice.

[lp:~mvo/click/coverage]

461. By Michael Vogt on 2014-06-09

fix get_executable() to only use python{,3}-coverage if its really in use

462. By Michael Vogt on 2014-06-09

use sys.exit(0) instead of return in the GIMOCK_SUBPROCESS code

[Michael Vogt (mvo)]

Thank you very much for the review Colin! I fixed the issues you raised now.

[Colin Watson (cjwatson)]

review: Approve

=== added file '.coveragerc'
--- .coveragerc	1970-01-01 00:00:00 +0000
+++ .coveragerc	2014-06-09 14:50:07 +0000
@@ -0,0 +1,2 @@
+[run]
+omit = /usr/lib/*, /usr/local/*, click/tests/*
\ No newline at end of file

=== modified file 'click/tests/__init__.py'
--- click/tests/__init__.py	2014-03-03 17:01:37 +0000
+++ click/tests/__init__.py	2014-06-09 14:50:07 +0000
@@ -16,6 +16,14 @@
     return True
 
 
+def get_executable():
+    """Get python executable (respecting if python-coverage was used)"""
+    coverage_executable = sys.executable+"-coverage"
+    if "coverage" in sys.modules and os.path.isfile(coverage_executable):
+        return [coverage_executable, "run", "-p"]
+    return [sys.executable]
+
+
 # Don't do any of this in interactive mode.
 if not hasattr(sys, "ps1"):
     _lib_click_dir = os.path.join(config.abs_top_builddir, "lib", "click")
@@ -26,6 +34,7 @@
     if _append_env_path("GI_TYPELIB_PATH", _lib_click_dir):
         changed = True
     if changed:
+        coverage_executable = get_executable()
         # We have to re-exec ourselves to get the dynamic loader to pick up
         # the new value of LD_LIBRARY_PATH.
         if "-m unittest" in sys.argv[0]:
@@ -33,7 +42,7 @@
             # "usefulness", making the re-exec more painful than it needs to
             # be.
             os.execvp(
-                sys.executable, [sys.executable, "-m", "unittest"] + sys.argv[1:])
+                coverage_executable[0], coverage_executable + ["-m", "unittest"] + sys.argv[1:])
         else:
-            os.execvp(sys.executable, [sys.executable] + sys.argv)
+            os.execvp(coverage_executable[0], coverage_executable + sys.argv)
         os._exit(1)

=== modified file 'click/tests/gimock.py'
--- click/tests/gimock.py	2014-03-10 09:30:31 +0000
+++ click/tests/gimock.py	2014-06-09 14:50:07 +0000
@@ -129,7 +129,7 @@
     import xml.etree.ElementTree as etree
 
 from click.tests.gimock_types import Stat, Stat64
-
+from click.tests import get_executable
 
 # Borrowed from giscanner.girparser.
 CORE_NS = "http://www.gtk.org/introspection/core/1.0"
@@ -188,6 +188,11 @@
         self._composite_refs = []
         self._delegate_funcs = {}
 
+    def doCleanups(self):
+        # we do not want to run the cleanups twice, just run it in the parent
+        if "GIMOCK_SUBPROCESS" not in os.environ:
+            return super(GIMockTestCase, self).doCleanups()
+
     def _gir_get_type(self, obj):
         ret = {}
         arrayinfo = obj.find(_corens("array"))
@@ -261,6 +266,7 @@
             "stdio.h",
             "stdint.h",
             "stdlib.h",
+            "string.h",
             "sys/types.h",
             "unistd.h",
             ])
@@ -292,6 +298,10 @@
                 conv["proto"] = ", ".join(
                     "%s %s" % pair for pair in zip(argtypes, argnames))
                 conv["args"] = ", ".join(argnames)
+                if conv["ret"] == "gchar*":
+                    conv["need_strdup"] = "strdup"
+                else:
+                    conv["need_strdup"] = ""
                 # The delegation scheme used here is needed because trying
                 # to pass pointers back and forward through ctypes is a
                 # recipe for having them truncated to 32 bits at the drop of
@@ -338,7 +348,7 @@
                                 delegate_%(name)s = 0;
                                 ret = (*ctypes_%(name)s) (%(args)s);
                                 if (! delegate_%(name)s)
-                                    return ret;
+                                    return %(need_strdup)s(ret);
                             }
                             return (*real_%(name)s) (%(args)s);
                         }
@@ -422,8 +432,8 @@
                 os.set_inheritable(wfd, True)
             else:
                 fcntl.fcntl(rfd, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
-            args = [
-                sys.executable, "-m", "unittest",
+            args = get_executable() + [
+                "-m", "unittest",
                 "%s.%s.%s" % (
                     self.__class__.__module__, self.__class__.__name__,
                     self._testMethodName)]
@@ -431,15 +441,25 @@
             env["GIMOCK_SUBPROCESS"] = str(wfd)
             if lib_path is not None:
                 env["LD_PRELOAD"] = lib_path
-            subp = subprocess.Popen(args, close_fds=False, env=env)
+            subp = subprocess.Popen(args, close_fds=False, env=env,
+                                    stdout=subprocess.PIPE,
+                                    stderr=subprocess.PIPE)
             os.close(wfd)
             reader = os.fdopen(rfd, "rb")
-            subp.communicate()
+            stdout, stderr = subp.communicate()
             exctype = pickle.load(reader)
-            if exctype is not None and issubclass(exctype, AssertionError):
+            # "normal" exit
+            if exctype is not None and issubclass(exctype, SystemExit):
+                pass
+            elif exctype is not None and issubclass(exctype, AssertionError):
+                print(stdout, file=sys.stdout)
+                print(stderr, file=sys.stderr)
                 raise AssertionError("Subprocess failed a test!")
             elif exctype is not None or subp.returncode != 0:
-                raise Exception("Subprocess returned an error!")
+                print(stdout, file=sys.stdout)
+                print(stderr, file=sys.stderr)
+                raise Exception("Subprocess returned an error! (%s, %s)" % (
+                    exctype, subp.returncode))
             reader.close()
             raise ParentProcess()
 
@@ -448,9 +468,11 @@
             if "GIMOCK_SUBPROCESS" in os.environ:
                 wfd = int(os.environ["GIMOCK_SUBPROCESS"])
                 writer = os.fdopen(wfd, "wb")
-                pickle.dump(None, writer)
+                # a sys.ext will generate a SystemExit exception, so we
+                # push it into the pipe so that the parent knows whats going on
+                pickle.dump(SystemExit, writer)
                 writer.flush()
-                os._exit(0)
+                sys.exit(0)
         except ParentProcess:
             pass
         except Exception as e:

=== modified file 'debian/control'
--- debian/control	2014-06-03 16:18:38 +0000
+++ debian/control	2014-06-09 14:50:07 +0000
@@ -3,7 +3,7 @@
 Priority: optional
 Maintainer: Colin Watson <[e-mail]>
 Standards-Version: 3.9.5
-Build-Depends: debhelper (>= 9~), dh-autoreconf, intltool, python3:any (>= 3.2), python3-all:any, python3-setuptools, python3-apt, python3-debian, python3-gi, python3:any (>= 3.3) | python3-mock, pep8, python3-pep8, pyflakes, python3-sphinx, pkg-config, valac, gobject-introspection (>= 0.6.7), libgirepository1.0-dev (>= 0.6.7), libglib2.0-dev (>= 2.34), gir1.2-glib-2.0, libjson-glib-dev (>= 0.10), libgee-0.8-dev, libpackagekit-glib2-dev (>= 0.7.2)
+Build-Depends: debhelper (>= 9~), dh-autoreconf, intltool, python3:any (>= 3.2), python3-all:any, python3-setuptools, python3-apt, python3-debian, python3-gi, python3:any (>= 3.3) | python3-mock, pep8, python3-pep8, pyflakes, python3-sphinx, pkg-config, valac, gobject-introspection (>= 0.6.7), libgirepository1.0-dev (>= 0.6.7), libglib2.0-dev (>= 2.34), gir1.2-glib-2.0, libjson-glib-dev (>= 0.10), libgee-0.8-dev, libpackagekit-glib2-dev (>= 0.7.2), python3-coverage
 Vcs-Bzr: https://code.launchpad.net/~ubuntu-managed-branches/click/click
 Vcs-Browser: http://bazaar.launchpad.net/~ubuntu-managed-branches/click/click/files
 X-Auto-Uploader: no-rewrite-version

=== modified file 'debian/rules'
--- debian/rules	2014-03-05 16:44:29 +0000
+++ debian/rules	2014-06-09 14:50:07 +0000
@@ -43,6 +43,12 @@
 	dh_auto_clean
 	$(MAKE) -C doc clean
 
+override_dh_auto_test:
+	python3-coverage run -m unittest discover click.tests
+	python3-coverage combine
+	python3-coverage xml
+	python3-coverage report
+
 PYTHON_INSTALL_FLAGS := \
 	--force --root=$(CURDIR)/debian/tmp \
 	--no-compile -O0 --install-layout=deb

=== modified file 'doc/index.rst'
--- doc/index.rst	2014-05-19 13:15:56 +0000
+++ doc/index.rst	2014-06-09 14:50:07 +0000
@@ -90,6 +90,13 @@
 
   $ python2 -m unittest click.tests.test_build.TestClickBuilder.test_build
 
+If you have python-coverage installed, you can get a test coverage report 
+by typing:
+
+  $ python-coverage combine
+  $ python-coverage report
+
+This works also for python3-coverage.
 
 
 Documentation