Merge lp:~cjwatson/click/fix-tests-installation into lp:click/devel

[Colin Watson]

Move integration tests to a location where they won't end up being installed into inappropriate places on the system module path (LP: #1337696).

The integration tests are being installed to /usr/lib/python3/dist-packages/tests/, which is clearly wrong. I moved them into click.tests to fix this, and put some additional arrangements in place so that they aren't run as part of unit tests.

This made the integration tests visible to pyflakes/pep8, which necessitated some follow-up tweaks.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:493
http://jenkins.qa.ubuntu.com/job/click-devel-ci/27/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/29/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/27/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/27/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/27/rebuild

review: Needs Fixing (continuous-integration)

[lp:~cjwatson/click/fix-tests-installation]

494. By Colin Watson on 2014-07-09

Hack around unittest.skipIf decorator ordering, which caused some bits of the integration test framework to try to run without sufficient dependencies installed.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:493
http://jenkins.qa.ubuntu.com/job/click-devel-ci/28/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/30/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/28/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/28/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/28/rebuild

review: Needs Fixing (continuous-integration)

[lp:~cjwatson/click/fix-tests-installation]

495. By Colin Watson on 2014-07-09

Don't fail if ping does not exist (as when running the unit tests in an environment without sufficient dependencies to run the integration tests).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:493
http://jenkins.qa.ubuntu.com/job/click-devel-ci/29/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/31
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/29
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/29/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/29

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/29/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:495
http://jenkins.qa.ubuntu.com/job/click-devel-ci/30/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/30
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/30/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/30

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/30/rebuild

review: Approve (continuous-integration)

[Michael Vogt (mvo)]

Thanks for this branch, it looks good, great that they are also covered by pep8/pyflakes now!
Some (small) inline comment below.

As a more general question: if the goal was to simply not install them, we could have changed setup.py to something like:
- packages=find_packages(),
+ packages=find_packages(exclude=["tests.integration"])),

But I assume moving the tests into the click/tests/integration subdir is also better from the code layout perspective? Moving it also opens up the opportunity to combine the click.tests.helper.TestCase with the one from the integration tests.

Thanks,
 Michael

review: Needs Information

[lp:~cjwatson/click/fix-tests-installation]

496. By Colin Watson on 2014-09-04

merge trunk

497. By Colin Watson on 2014-09-04

More PEP-8 failures.

498. By Colin Watson on 2014-09-04

Call skipTest in ClickTestCase.setUp rather than using allow_integration helper everywhere.

499. By Colin Watson on 2014-09-04

Use is_root helper in another place.

500. By Colin Watson on 2014-09-04

Rearrange integration test skip logic to do everything in setUpClass rather than via decorators, for clarity of ordering.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:500
http://jenkins.qa.ubuntu.com/job/click-devel-ci/55/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/55
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/55/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/55

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/55/rebuild

review: Approve (continuous-integration)

[Colin Watson (cjwatson)]

Thanks; I've addressed your comments.

I indeed felt it better to have the tests live under click/tests/. It's true that we could probably now consolidate some duplicated code, but I think that probably ought to be done in a separate branch.

[Michael Vogt (mvo)]

Thanks, this looks great.

review: Approve

=== renamed directory 'tests/integration' => 'click/tests/integration'
=== modified file 'click/tests/integration/helpers.py'
--- tests/integration/helpers.py	2014-08-03 13:10:48 +0000
+++ click/tests/integration/helpers.py	2014-09-04 21:07:10 +0000
@@ -27,13 +27,17 @@
 import unittest
 
 
-def is_root():
-    return os.getuid() == 0
-
-
-def has_network():
-    return subprocess.call(
-        ["ping", "-c1", "archive.ubuntu.com"]) == 0
+def require_root():
+    if os.getuid() != 0:
+        raise unittest.SkipTest("This test needs to run as root")
+
+
+def require_network():
+    try:
+        if subprocess.call(["ping", "-c1", "archive.ubuntu.com"]) != 0:
+            raise unittest.SkipTest("Need network")
+    except Exception:
+        pass
 
 
 @contextlib.contextmanager
@@ -50,6 +54,8 @@
 
     @classmethod
     def setUpClass(cls):
+        if "TEST_INTEGRATION" not in os.environ:
+            raise unittest.SkipTest("Skipping integration tests")
         cls.click_binary = os.environ.get("CLICK_BINARY", "/usr/bin/click")
 
     def setUp(self):

=== modified file 'click/tests/integration/test_build_core_apps.py'
--- tests/integration/test_build_core_apps.py	2014-08-03 13:10:48 +0000
+++ click/tests/integration/test_build_core_apps.py	2014-09-04 21:07:10 +0000
@@ -20,12 +20,13 @@
 import os
 import shutil
 import subprocess
-import unittest
+
+from six import with_metaclass
 
 from .helpers import (
     chdir,
-    has_network,
-    is_root,
+    require_network,
+    require_root,
     ClickTestCase,
 )
 
@@ -63,9 +64,13 @@
         return type.__new__(cls, name, bases, dct)
 
 
[e-mail](not is_root(), "This tests needs to run as root")
[e-mail](not has_network(), "Need network")
-class TestBuildCoreApps(ClickTestCase, metaclass=AddBranchTestFunctions):
+class TestBuildCoreApps(with_metaclass(AddBranchTestFunctions, ClickTestCase)):
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestBuildCoreApps, cls).setUpClass()
+        require_root()
+        require_network()
 
     def _run_in_chroot(self, cmd):
         """Run the given cmd in a click chroot"""

=== modified file 'click/tests/integration/test_chroot.py'
--- tests/integration/test_chroot.py	2014-08-03 13:10:48 +0000
+++ click/tests/integration/test_chroot.py	2014-09-04 21:07:10 +0000
@@ -16,22 +16,21 @@
 """Integration tests for the click chroot feature."""
 
 import subprocess
-import unittest
 
 from .helpers import (
-    has_network,
-    is_root,
+    require_network,
+    require_root,
     ClickTestCase,
 )
 
 
[e-mail](not is_root(), "This tests needs to run as root")
[e-mail](not has_network(), "Need network")
 class TestChroot(ClickTestCase):
 
     @classmethod
     def setUpClass(cls):
         super(TestChroot, cls).setUpClass()
+        require_root()
+        require_network()
         cls.arch = subprocess.check_output(
             ["dpkg", "--print-architecture"], universal_newlines=True).strip()
         subprocess.check_call([

=== modified file 'click/tests/integration/test_frameworks.py'
--- tests/integration/test_frameworks.py	2014-06-26 12:00:09 +0000
+++ click/tests/integration/test_frameworks.py	2014-09-04 21:07:10 +0000
@@ -17,18 +17,17 @@
 
 import os
 import subprocess
-import unittest
-
-from .helpers import (
-    ClickTestCase,
-)
-
-
[e-mail](
-    (not os.path.exists("/usr/share/click/frameworks") or
-     not os.listdir("/usr/share/click/frameworks")),
-    "Please install ubuntu-sdk-libs")
+
+from .helpers import ClickTestCase
+
+
 class TestFrameworks(ClickTestCase):
+    def setUp(self):
+        super(TestFrameworks, self).setUp()
+        if (not os.path.exists("/usr/share/click/frameworks") or
+                not os.listdir("/usr/share/click/frameworks")):
+            self.skipTest("Please install ubuntu-sdk-libs")
+
     def test_framework_list(self):
         output = subprocess.check_output([
             self.click_binary, "framework", "list"], universal_newlines=True)

=== modified file 'click/tests/integration/test_hook.py'
--- tests/integration/test_hook.py	2014-08-22 17:01:08 +0000
+++ click/tests/integration/test_hook.py	2014-09-04 21:07:10 +0000
@@ -18,16 +18,20 @@
 import os
 import subprocess
 from textwrap import dedent
-import unittest
 
 from .helpers import (
-    is_root,
     ClickTestCase,
+    require_root,
 )
 
 
[e-mail](not is_root(), "This tests needs to run as root")
 class TestHook(ClickTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestHook, cls).setUpClass()
+        require_root()
+
     def _make_hook(self, name):
         hook_fname = "/usr/share/click/hooks/%s.hook" % name
         canary_fname = os.path.join(self.temp_dir, "canary.sh")

=== modified file 'click/tests/integration/test_install.py'
--- tests/integration/test_install.py	2014-08-22 17:01:08 +0000
+++ click/tests/integration/test_install.py	2014-09-04 21:07:10 +0000
@@ -15,11 +15,12 @@
 
 """Integration tests for the click install feature."""
 
-import os
 import subprocess
-import unittest
 
-from .helpers import ClickTestCase
+from .helpers import (
+    require_root,
+    ClickTestCase,
+)
 
 
 def add_user(name):
@@ -31,13 +32,12 @@
     subprocess.check_call(["userdel", "-r", name])
 
 
[e-mail](
-    os.getuid() != 0, "This tests needs to run as root")
 class TestClickInstall(ClickTestCase):
 
     @classmethod
     def setUpClass(cls):
         super(TestClickInstall, cls).setUpClass()
+        require_root()
         cls.USER_1 = add_user("click-test-user-1")
         cls.USER_2 = add_user("click-test-user-2")
 

=== modified file 'click/tests/integration/test_signatures.py'
--- tests/integration/test_signatures.py	2014-08-22 17:01:08 +0000
+++ click/tests/integration/test_signatures.py	2014-09-04 21:07:10 +0000
@@ -20,20 +20,21 @@
 import shutil
 import subprocess
 import tarfile
-import unittest
 from textwrap import dedent
 
 from .helpers import (
-    is_root,
+    require_root,
     ClickTestCase,
 )
 
+
 def makedirs(path):
     try:
         os.makedirs(path)
     except OSError:
         pass
 
+
 def get_keyid_from_gpghome(gpg_home):
     """Return the public keyid of a given gpg home dir"""
     output = subprocess.check_output(
@@ -56,7 +57,7 @@
     def sign(self, filepath, signature_type="origin"):
         """Sign the click at filepath"""
         env = copy.copy(os.environ)
-        env["GNUPGHOME"] = os.path.abspath(self.gpghome)        
+        env["GNUPGHOME"] = os.path.abspath(self.gpghome)
         subprocess.check_call(
             ["debsigs",
              "--sign=%s" % signature_type,
@@ -74,7 +75,7 @@
         <Selection>
         <Required Type="origin" File="{filename}" id="{keyid}"/>
         </Selection>
-  
+
         <Verification>
         <Required Type="origin" File="{filename}" id="{keyid}"/>
         </Verification>
@@ -86,7 +87,8 @@
         self.pubkey_path = (
             "/usr/share/debsig/keyrings/%s/origin.pub" % self.keyid)
         makedirs(os.path.dirname(self.pubkey_path))
-        shutil.copy(os.path.join(self.gpghome, "pubring.gpg"), self.pubkey_path)
+        shutil.copy(
+            os.path.join(self.gpghome, "pubring.gpg"), self.pubkey_path)
 
     def uninstall_signature_policy(self):
         # FIXME: update debsig-verify so that it can work from a different
@@ -96,8 +98,13 @@
         os.remove(self.pubkey_path)
 
 
[e-mail](not is_root(), "This tests needs to run as root")
 class ClickSignaturesTestCase(ClickTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super(ClickSignaturesTestCase, cls).setUpClass()
+        require_root()
+
     def assertClickNoSignatureError(self, cmd_args):
         with self.assertRaises(subprocess.CalledProcessError) as cm:
             output = subprocess.check_output(
@@ -120,8 +127,13 @@
         self.assertIn(expected_error_message, output)
 
 
[e-mail](not is_root(), "This tests needs to run as root")
 class TestSignatureVerificationNoSignature(ClickSignaturesTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestSignatureVerificationNoSignature, cls).setUpClass()
+        require_root()
+
     def test_debsig_verify_no_sig(self):
         name = "org.example.debsig-no-sig"
         path_to_click = self._make_click(name, framework="")
@@ -145,8 +157,13 @@
                               "--user=%s" % user, name])
 
 
[e-mail](not is_root(), "This tests needs to run as root")
 class TestSignatureVerification(ClickSignaturesTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestSignatureVerification, cls).setUpClass()
+        require_root()
+
     def setUp(self):
         super(TestSignatureVerification, self).setUp()
         self.user = os.environ.get("USER", "root")
@@ -176,7 +193,7 @@
             [self.click_binary, "list", "--user=%s" % self.user],
             universal_newlines=True)
         self.assertIn(name, output)
-        
+
     def test_debsig_install_signature_not_in_keyring(self):
         name = "org.example.debsig-no-keyring-sig"
         path_to_click = self._make_click(name, framework="")
@@ -245,7 +262,7 @@
         # NOTE: that right now this will not be caught by debsig-verify
         #        but later in audit() by debian.debfile.DebFile()
         subprocess.check_call(["ar",
-                               "-r", 
+                               "-r",
                                "-b", "data.tar.gz",
                                path_to_click,
                                new_data])
@@ -274,12 +291,12 @@
         new_data = self.make_nasty_data_tar("bz2")
         # replace data.tar.gz with data.tar.bz2 and ensure this is caught
         subprocess.check_call(["ar",
-                               "-d", 
+                               "-d",
                                path_to_click,
                                "data.tar.gz",
                                ])
         subprocess.check_call(["ar",
-                               "-r", 
+                               "-r",
                                path_to_click,
                                new_data])
         output = subprocess.check_output(
@@ -290,7 +307,7 @@
                           "control.tar.gz",
                           "_gpgorigin",
                           "data.tar.bz2",
-                         ])
+                          ])
         with self.assertRaises(subprocess.CalledProcessError) as cm:
             output = subprocess.check_output(
                 [self.click_binary, "install", path_to_click],
@@ -305,7 +322,7 @@
         # this test is probably not really needed, it tries to trick
         # the system by prepending a valid signature that is not
         # in the keyring. But given that debsig-verify only reads
-        # the first packet of any given _gpg$foo signature its 
+        # the first packet of any given _gpg$foo signature it's
         # equivalent to test_debsig_install_signature_not_in_keyring test
         name = "org.example.debsig-replaced-data-prepend-sig-click"
         path_to_click = self._make_click(name, framework="")
@@ -313,7 +330,7 @@
         new_data = self.make_nasty_data_tar("gz")
         # replace data.tar.gz
         subprocess.check_call(["ar",
-                               "-r", 
+                               "-r",
                                path_to_click,
                                new_data,
                                ])
@@ -345,4 +362,3 @@
             [self.click_binary, "list", "--user=%s" % self.user],
             universal_newlines=True)
         self.assertNotIn(name, output)
-        

=== modified file 'debian/changelog'
--- debian/changelog	2014-09-02 08:35:59 +0000
+++ debian/changelog	2014-09-04 21:07:10 +0000
@@ -1,3 +1,13 @@
+click (0.4.32) UNRELEASED; urgency=medium
+
+  * Move integration tests to a location where they won't end up being
+    installed into inappropriate places on the system module path
+    (LP: #1337696).
+  * Use six.with_metaclass for TestBuildCoreApps, so that it doesn't make
+    pyflakes angry when running tests under Python 2.
+
+ -- Colin Watson <[e-mail]>  Thu, 04 Sep 2014 14:46:16 +0100
+
 click (0.4.31.3) utopic; urgency=medium
 
   * Reinstate most of 0.4.31.2; instead, just always pass

=== modified file 'debian/control'
--- debian/control	2014-06-10 17:18:00 +0000
+++ debian/control	2014-09-04 21:07:10 +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), python3-coverage
+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, python3-six
 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/tests/control'
--- debian/tests/control	2014-07-14 09:10:39 +0000
+++ debian/tests/control	2014-09-04 21:07:10 +0000
@@ -1,3 +1,3 @@
 Tests: run-tests.sh
-Depends: @, iputils-ping, click-dev, schroot, debootstrap, sudo, bzr, debsigs, debsig-verify
+Depends: @, iputils-ping, click-dev, schroot, debootstrap, sudo, bzr, debsigs, debsig-verify, python3-six
 Restrictions: needs-root allow-stderr

=== modified file 'debian/tests/run-tests.sh'
--- debian/tests/run-tests.sh	2014-06-11 12:40:44 +0000
+++ debian/tests/run-tests.sh	2014-09-04 21:07:10 +0000
@@ -2,4 +2,4 @@
 
 set -e
 
-python3 -m unittest discover tests.integration
+TEST_INTEGRATION=1 python3 -m unittest discover -vv click.tests.integration

=== removed directory 'tests'
=== removed file 'tests/__init__.py'