Merge ppa-dev-tools:improve-ppa-address-handling into ppa-dev-tools:main

[Bryce Harrington]

Fixes some reported bugs relating to handling of the ppa-address argument to various commands.

This improves handling of cases where no team is specified (and thus should default appropriately to the current user's namespace), and catching a variety of invalid team and ppa names. A slew of test cases are added for this.

Of note, this also adds the ability to pass in a PPA url, in addition to a plain name or a formal ppa:foo/bar style address. I think this should make the commands a bit more handy.

8b8bb77... by Bryce Harrington on 2022-10-18

README: Use formal ppa address in docs

With the recent fix, all the steps that were documented in README should
now work as written. However, using the formal ppa address is more
generally correct so should be preferred in the entry-level docs.

[Lena Voytek (lvoytek)]

LGTM, just some small code cleanup comments. All logic looks good

review: Approve

[Bryce Harrington (bryce)]

I've incorporated fixes for all the feedback (and filed a bug for one), and landed the branch:

To git+ssh://git.launchpad.net/ppa-dev-tools
   222405e..e5b49d1 main -> main

Thank you again for the review, Lena!

diff --git a/README.md b/README.md
index 4b5d34d..d26e1a4 100644
--- a/README.md
+++ b/README.md
@@ -43,7 +43,7 @@ $ dput ppa:my-name/my-ppa some-package.changes
 Wait until all packages in the PPA have finished building
 
 ```
-$ ppa wait my-ppa
+$ ppa wait ppa:my-name/my-ppa
 ```
 
 Set the public description for a PPA from a file
@@ -55,5 +55,5 @@ $ cat some-package/README | ppa desc ppa:my-name/my-ppa
 Delete the PPA
 
 ```
-$ ppa destroy my-ppa
+$ ppa destroy ppa:my-name/my-ppa
 ```
diff --git a/ppa/ppa.py b/ppa/ppa.py
index 2617346..888df48 100755
--- a/ppa/ppa.py
+++ b/ppa/ppa.py
@@ -8,6 +8,8 @@
 # Released under GNU GPLv2 or later, read the file 'LICENSE.GPLv2+' for
 # more information.
 
+import re
+
 from textwrap import indent
 from functools import lru_cache
 from lazr.restfulclient.errors import BadRequest, NotFound
@@ -17,7 +19,8 @@ class PpaDoesNotExist(BaseException):
     """Exception indicating a requested PPA could not be found."""
 
     def __init__(self, ppa_name, team_name, message=None):
-        """
+        """Initializes the exception object.
+
         :param str ppa_name: The name of the missing PPA.
         :param str message: An error message.
         """
@@ -36,40 +39,6 @@ class PpaDoesNotExist(BaseException):
         return f"The PPA '{self.ppa_name}' does not exist for team or user '{self.team_name}'"
 
 
-def ppa_address_split(ppa_address, default_team='me'):
-    """Parse an address for a PPA into its team and name components
-    """
-    if ppa_address.startswith('ppa:'):
-        if '/' not in ppa_address:
-            return (None, None)
-        rem = ppa_address.split('ppa:', 1)[1]
-        team_name = rem.split('/', 1)[0]
-        ppa_name = rem.split('/', 1)[1]
-    else:
-        team_name = default_team
-        ppa_name = ppa_address
-    return (team_name, ppa_name)
-
-
-def get_das(distro, series_name, arch_name):
-    """Retrive the arch-series for the given distro.
-
-    :param distribution distro: The Launchpad distribution object.
-    :param str series_name: The distro's codename for the series.
-    :param str arch_name: The hardware architecture.
-    :rtype: distro_arch_series
-    :returns: A Launchpad distro_arch_series object, or None on error.
-    """
-    if series_name is None or series_name == '':
-        return None
-
-    for series in distro.series:
-        if series.name != series_name:
-            continue
-        return series.getDistroArchSeries(archtag=arch_name)
-    return None
-
-
 class Ppa:
     """Encapsulates data needed to access and conveniently wrap a PPA
 
@@ -77,7 +46,7 @@ class Ppa:
     of data from the remote.
     """
     def __init__(self, ppa_name, team_name, ppa_description=None, service=None):
-        """Initializes a new Ppa object for a given PPA
+        """Initializes a new Ppa object for a given PPA.
 
         This creates only the local representation of the PPA, it does
         not cause a new PPA to be created in Launchpad.  For that, see
@@ -102,6 +71,11 @@ class Ppa:
         self._service = service
 
     def __repr__(self):
+        """Machine-parsable unique representation of object.
+
+        :rtype: str
+        :returns: Official string representation of the object.
+        """
         return (f'{self.__class__.__name__}('
                 f'ppa_name={self.ppa_name!r}, team_name={self.team_name!r})')
 
@@ -130,7 +104,7 @@ class Ppa:
     @property
     @lru_cache
     def address(self):
-        """The proper identifier of the PPA
+        """The proper identifier of the PPA.
 
         :rtype: str
         :returns: The full identification string for the PPA.
@@ -371,3 +345,77 @@ class Ppa:
         if not retval:
             print("Successfully published all builds for all architectures")
         return retval
+
+
+def ppa_address_split(ppa_address, default_team=None):
+    """Parse an address for a PPA into its team and name components.
+
+    :param str ppa_address: A ppa name or address.
+    :param str default_team: (Optional) name of team to use if missing.
+    :rtype: tuple(str, str)
+    :returns: The team name and ppa name as a tuple, or (None, None) on error.
+    """
+    if not ppa_address or len(ppa_address)<2:
+        return (None, None)
+    if ppa_address.startswith('ppa:'):
+        if '/' not in ppa_address:
+            return (None, None)
+        rem = ppa_address.split('ppa:', 1)[1]
+        team_name = rem.split('/', 1)[0]
+        ppa_name = rem.split('/', 1)[1]
+    elif ppa_address.startswith('http'):
+        # Only launchpad PPA urls are supported
+        m = re.search(r'https:\/\/launchpad\.net\/~([^/]+)\/\+archive\/ubuntu\/(.+)$', ppa_address)
+        if not m:
+            return (None, None)
+        team_name = m.group(1)
+        ppa_name = m.group(2)
+    elif '/' in ppa_address:
+        team_name = ppa_address.split('/', 1)[0]
+        ppa_name = ppa_address.split('/', 1)[1]
+    else:
+        team_name = default_team
+        ppa_name = ppa_address
+
+    if (team_name and ppa_name
+        and not (any(x.isupper() for x in team_name))
+        and not (any(x.isupper() for x in ppa_name))
+        and ppa_name.isascii()
+        and '/' not in ppa_name
+        and len(ppa_name)>1):
+        return (team_name, ppa_name)
+
+    return (None, None)
+
+
+def get_das(distro, series_name, arch_name):
+    """Retrive the arch-series for the given distro.
+
+    :param distribution distro: The Launchpad distribution object.
+    :param str series_name: The distro's codename for the series.
+    :param str arch_name: The hardware architecture.
+    :rtype: distro_arch_series
+    :returns: A Launchpad distro_arch_series object, or None on error.
+    """
+    if series_name is None or series_name == '':
+        return None
+
+    for series in distro.series:
+        if series.name != series_name:
+            continue
+        return series.getDistroArchSeries(archtag=arch_name)
+    return None
+
+
+def get_ppa(lp, config):
+    """Load the specified PPA from Launchpad
+
+    :param Lp lp: The Launchpad wrapper object.
+    :param dict config: Configuration param:value map.
+    :rtype: Ppa
+    :returns: Specified PPA as a Ppa object.
+    """
+    return Ppa(
+        ppa_name=config.get('ppa_name', None),
+        team_name=config.get('team_name', None),
+        service=lp)
diff --git a/ppa/ppa_group.py b/ppa/ppa_group.py
index 1a9c8b7..0160574 100755
--- a/ppa/ppa_group.py
+++ b/ppa/ppa_group.py
@@ -16,10 +16,11 @@ from lazr.restfulclient.errors import BadRequest
 
 
 class PpaAlreadyExists(BaseException):
-    '''Exception indicating a PPA operation could not be performed'''
+    """Exception indicating a PPA operation could not be performed."""
 
     def __init__(self, ppa_name, message=None):
-        """
+        """Initializes the exception object.
+
         :param str ppa_name: The name of the pre-existing PPA.
         :param str message: An error message.
         """
@@ -44,25 +45,31 @@ class PpaGroup:
     This class provides a proxy object for interacting with collections
     of PPA.
     """
-    def __init__(self, service, name='me'):
-        """
+    def __init__(self, service, name):
+        """Initializes a new PpaGroup object for a named person or team.
+
         :param launchpadlib.service service: The Launchpad service object.
-        :param str name: Launchpad username
+        :param str name: Launchpad team or user name
         """
-        assert(service is not None)
+        assert service is not None
         self.service = service
-
-        if name == 'me':
-            me = self.service.me
-            self.name = me.name
-        else:
-            self.name = name
+        self.name = name
 
     def __repr__(self):
+        """Machine-parsable unique representation of object.
+
+        :rtype: str
+        :returns: Official string representation of the object.
+        """
         return (f'{self.__class__.__name__}('
                 f'service={self.service!r}, name={self.name!r})')
 
     def __str__(self):
+        """Human-readable summary of the object.
+
+        :rtype: str
+        :returns: Printable summary of the object.
+        """
         return 'tbd'
 
     @property
@@ -70,8 +77,8 @@ class PpaGroup:
     def team(self):
         """The team that owns this collection of PPAs.
 
-        :rtype: tbd
-        :returns: tbd
+        :rtype: launchpadlib.person
+        :returns: Launchpad person object that owns this PPA.
         """
         return self.service.people[self.name]
 
diff --git a/scripts/ppa b/scripts/ppa
index 361d91b..6965d44 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -77,7 +77,12 @@ from ppa.job import (
     show_running
 )
 from ppa.lp import Lp
-from ppa.ppa import Ppa, PpaDoesNotExist
+from ppa.ppa import (
+    get_ppa,
+    ppa_address_split,
+    Ppa,
+    PpaDoesNotExist
+)
 from ppa.ppa_group import PpaGroup, PpaAlreadyExists
 from ppa.result import (
     Result,
@@ -87,7 +92,7 @@ from ppa.text import o2str
 from ppa.trigger import Trigger
 
 import ppa.debug
-from ppa.debug import dbg, die, warn
+from ppa.debug import dbg, warn
 
 
 def UNIMPLEMENTED():
@@ -197,7 +202,7 @@ def create_arg_parser():
     return parser
 
 
-def create_config(args):
+def create_config(lp, args):
     """Creates config object by loading from file and adding args.
 
     This routine merges the command line parameter values with data
@@ -208,6 +213,7 @@ def create_config(args):
     This permits setting static values in the config file(s), and using
     the command line args for variable settings and overrides.
 
+    :param launchpadlib.service lp: The Launchpad service object.
     :param Namespace args: The parsed args from ArgumentParser.
     :rtype: dict
     :returns: dict of configuration parameters and values, or None on error
@@ -233,21 +239,12 @@ def create_config(args):
     for k, v in DEFAULT_CONFIG.items():
         config.setdefault(k, v)
 
-    # TODO: function to convert string to ppa_name, team_name
-    if args.ppa_name.startswith('ppa:'):
-        if '/' not in args.ppa_name:
-            die("Invalid ppa name '{}'".format(args.ppa_name))
-        rem = args.ppa_name.split('ppa:', 1)[1]
-        config['team_name'] = rem.split('/', 1)[0]
-        config['ppa_name'] = rem.split('/', 1)[1]
-    elif '/' in args.ppa_name:
-        config['team_name'] = args.ppa_name.split('/', 1)[0]
-        config['ppa_name'] = args.ppa_name.split('/', 1)[1]
-    else:
-        config['ppa_name'] = args.ppa_name
-        # TODO: Set team_name to current lp_username if available
-    if 'ppa_name' not in config or 'team_name' not in config:
-        warn("Unknown ppa or team name")
+    lp_username = None
+    if lp.me:
+        lp_username = lp.me.name
+    config['team_name'], config['ppa_name'] = ppa_address_split(args.ppa_name, lp_username)
+    if not config['team_name'] or not config['ppa_name']:
+        warn("Invalid ppa name '{}'".format(args.ppa_name))
         return None
 
     if args.dry_run:
@@ -263,20 +260,6 @@ def create_config(args):
     return config
 
 
-def get_ppa(lp, config):
-    """Load the specified PPA from Launchpad
-
-    :param Lp lp: The Launchpad wrapper object.
-    :param dict config: Configuration param:value map.
-    :rtype: Ppa
-    :returns: Specified PPA as a Ppa object.
-    """
-    return Ppa(
-        ppa_name=config.get('ppa_name', None),
-        team_name=config.get('team_name', None),
-        service=lp)
-
-
 ################
 ### Commands ###
 ################
@@ -296,12 +279,13 @@ def command_create(lp, config):
 
     ppa_name = config.get('ppa_name')
     if not ppa_name:
-        warn("Could not determine ppa_name")
+        warn("Could not determine PPA name")
         return os.EX_USAGE
 
     team_name = config.get('team_name')
     if not team_name:
-        team_name = 'me'
+        warn("Could not determine team name")
+        return os.EX_USAGE
 
     architectures = config.get('architectures', ARCHES_PPA)
 
@@ -404,8 +388,13 @@ def command_list(lp, config, filter_func=None):
     if not lp:
         return 1
 
+    team_name = config.get('team_name')
+    if not team_name:
+        warn("Could not determine team name")
+        return os.EX_USAGE
+
     try:
-        ppa_group = PpaGroup(service=lp)
+        ppa_group = PpaGroup(service=lp, name=team_name)
         for ppa in ppa_group.ppas:
             print(ppa.address)
         return os.EX_OK
@@ -629,8 +618,9 @@ def main(args):
     """
     config = create_config(args)
     if not config:
-        parser.error("Invalid parameters")
-        return 1
+        return os.EX_CONFIG
+
+    ppa.debug.DEBUGGING = config.get('debug', False)
     dbg("Configuration:")
     dbg(config)
 
diff --git a/tests/helpers.py b/tests/helpers.py
new file mode 100644
index 0000000..c774cfc
--- /dev/null
+++ b/tests/helpers.py
@@ -0,0 +1,90 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+# Author:  Bryce Harrington <[e-mail]>
+#
+# Copyright (C) 2019 Bryce W. Harrington
+#
+# Released under GNU GPLv2 or later, read the file 'LICENSE.GPLv2+' for
+# more information.
+
+import os
+import sys
+
+sys.path.insert(0, os.path.realpath(
+	os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
+
+from ppa.ppa import Ppa
+from ppa.ppa_group import PpaGroup, PpaAlreadyExists
+
+
+class PersonMock:
+    """A stand-in for a Launchpad Person object."""
+    def __init__(self, name):
+        self.name = name
+        self._ppas = []
+
+    def createPPA(self, name, description, displayname):
+        for ppa in self._ppas:
+            if ppa.name == name:
+                raise PpaAlreadyExists(name)
+        new_ppa = Ppa(name, self.name, description)
+        self._ppas.append(new_ppa)
+        return True
+
+    def lp_save(self):
+        return True
+
+    @property
+    def ppas(self):
+        return self._ppas
+
+
+class LaunchpadMock:
+    """A stand-in for Launchpad."""
+    def __init__(self):
+        self.people = { 'me': PersonMock('me') }
+
+    def add_person(self, name):
+        print("Adding person %s" %(name))
+        self.people[name] = PersonMock(name)
+
+    @property
+    def me(self):
+        return self.people['me']
+
+
+class LpServiceMock:
+    """A stand-in for the Lp service object."""
+    def __init__(self):
+        self.launchpad = LaunchpadMock()
+
+    @property
+    def me(self):
+        return self.launchpad.people['me']
+
+    @property
+    def people(self):
+        return self.launchpad.people
+
+    def get_bug(self, bug_id):
+        class BugMock:
+            @property
+            def title(self):
+                return "Mock bug report"
+
+            @property
+            def description(self):
+                return "Description line 1\n\ndescription line 2"
+
+        return BugMock()
+
+
+class RequestResponseMock:
+    """A stand-in for a request result."""
+    def __init__(self, text):
+        self._text = text.encode('utf-8')
+
+    def read(self):
+        """Simply returns the exact text provided in initializer."""
+        return self._text
diff --git a/tests/test_io.py b/tests/test_io.py
index 57f8737..c15d2af 100644
--- a/tests/test_io.py
+++ b/tests/test_io.py
@@ -15,13 +15,16 @@ import sys
 import urllib
 
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.io import open_url
 
 
 def test_open_url(tmp_path):
-    """Checks that the open_url() object reads from a valid URL."""
+    """Checks that the open_url() object reads from a valid URL.
+
+    :param fixture tmp_path: Temp dir.
+    """
     f = tmp_path / "open_url.txt"
     f.write_text("abcde")
 
diff --git a/tests/test_job.py b/tests/test_job.py
index 44491c2..78a12c7 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -14,19 +14,10 @@ import os
 import sys
 
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.job import Job, get_running, get_waiting
-
-
-class ResponseMock:
-    """Synthetic response object"""
-    def __init__(self, text):
-        self._text = text.encode('utf-8')
-
-    def read(self):
-        """Simply returns the exact text provided in initializer."""
-        return self._text
+from tests.helpers import RequestResponseMock
 
 
 def test_object():
@@ -81,7 +72,7 @@ def test_request_url():
 
 
 def test_get_running():
-    """Checks output from the get_running() command"""
+    """Checks output from the get_running() command."""
     json_text = ('{"mypackage": {"my-job-id": {"focal": { "arm64": ['
                  '{"submit-time": "2022-08-19 20:59:01", '
                  '"triggers": ["yourpackage/1.2.3"], '
@@ -89,7 +80,7 @@ def test_get_running():
                  '1234, '
                  '"Log Output Here"'
                  '] } } } }')
-    fake_response = ResponseMock(json_text)
+    fake_response = RequestResponseMock(json_text)
     job = next(get_running(fake_response, series='focal', ppa='ppa:me/myppa'))
     assert repr(job) == "Job(source_package='mypackage', series='focal', arch='arm64')"
     assert job.triggers == ["yourpackage/1.2.3"]
@@ -97,7 +88,7 @@ def test_get_running():
 
 
 def test_get_waiting():
-    """Checks output from the get_waiting() command"""
+    """Checks output from the get_waiting() command."""
     # TODO: I think ppas need to be in "ppa" instead of under "ubuntu" but need to doublecheck.
     json_text = ('{ "ubuntu": { "focal": { "amd64": ['
                  ' "a\\n{\\"requester\\":   \\"you\\",'
@@ -108,7 +99,7 @@ def test_get_waiting():
                  '      \\"ppas\\":        [ \\"ppa:me/myppa\\" ],'
                  '      \\"triggers\\":    [ \\"c/3.2-1\\", \\"d/2-2\\" ] }"'
                  '] } } }')
-    fake_response = ResponseMock(json_text)
+    fake_response = RequestResponseMock(json_text)
     job = next(get_waiting(fake_response, series='focal', ppa='ppa:me/myppa'))
     assert job
     assert job.source_package == "b"
diff --git a/tests/test_lp.py b/tests/test_lp.py
index d5978ce..98f4888 100644
--- a/tests/test_lp.py
+++ b/tests/test_lp.py
@@ -34,7 +34,8 @@ from mock import Mock
 import pytest
 import logging
 
-sys.path.insert(0, os.path.realpath(os.path.join(os.path.dirname(__file__), "..")))
+sys.path.insert(0, os.path.realpath(
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.lp import Lp
 from launchpadlib.launchpad import Launchpad
diff --git a/tests/test_ppa.py b/tests/test_ppa.py
index a71336f..2410f83 100644
--- a/tests/test_ppa.py
+++ b/tests/test_ppa.py
@@ -11,26 +11,69 @@
 import os
 import sys
 
+import pytest
+
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
-from ppa.ppa import Ppa
+from ppa.ppa import Ppa, ppa_address_split, get_ppa
 
 
 def test_object():
-    """Check that PPA objects can be instantiated"""
+    """Check that PPA objects can be instantiated."""
     ppa = Ppa('test-ppa-name', 'test-team-name')
     assert ppa
 
 
 def test_description():
-    """Check specifying a description when creating a PPA"""
+    """Check specifying a description when creating a PPA."""
     ppa = Ppa('test-ppa-name', 'test-team-name', 'test-description')
 
     assert 'test-description' in ppa.ppa_description
 
 
 def test_address():
-    """Check getting the PPA address"""
+    """Check getting the PPA address."""
     ppa = Ppa('test', 'team')
     assert ppa.address == "ppa:team/test"
+
+
[e-mail]('address, default_team, expected', [
+    # Successful cases
+    ('bb', 'me', ('me', 'bb')),
+    ('123', 'me', ('me', '123')),
+    ('a/123', 'me', ('a', '123')),
+    ('ppa:a/bb', None, ('a', 'bb')),
+    ('ppa:ç/bb', None, ('ç', 'bb')),
+    ('https://launchpad.net/~a/+archive/ubuntu/bb', None, ('a', 'bb')),
+
+    # Expected failure cases
+    ('ppa:', None, (None, None)),
+    (None, None, (None, None)),
+    ('', None, (None, None)),
+    ('/', None, (None, None)),
+    (':/', None, (None, None)),
+    ('////', None, (None, None)),
+    ('ppa:/', None, (None, None)),
+    ('ppa:a/', None, (None, None)),
+    ('ppa:/bb', None, (None, None)),
+    ('ppa:a/bç', None, (None, None)),
+    ('ppa:A/bb', None, (None, None)),
+    ('ppa/a/bb', None, (None, None)),
+    ('ppa:a/bb/c', None, (None, None)),
+    ('ppa:a/bB', None, (None, None)),
+    ('http://launchpad.net/~a/+archive/ubuntu/bb', None, (None, None)),
+    ('https://example.com/~a/+archive/ubuntu/bb', None, (None, None)),
+    ('https://launchpad.net/~a/+archive/nobuntu/bb', None, (None, None)),
+])
+def test_ppa_address_split(address, default_team, expected):
+    """Check ppa address input strings can be parsed properly."""
+    result = ppa_address_split(address, default_team=default_team)
+    assert result == expected
+
+
+def test_get_ppa():
+    ppa = get_ppa(None, {'team_name': 'a', 'ppa_name': 'bb'})
+    assert type(ppa) is Ppa
+    assert ppa.team_name == 'a'
+    assert ppa.ppa_name == 'bb'
diff --git a/tests/test_ppa_group.py b/tests/test_ppa_group.py
index e03a841..63907fd 100644
--- a/tests/test_ppa_group.py
+++ b/tests/test_ppa_group.py
@@ -14,141 +14,83 @@ import sys
 import pytest
 
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.ppa import Ppa
 from ppa.ppa_group import PpaGroup, PpaAlreadyExists
-
-
-class PersonMock:
-    def __init__(self, name):
-        self.name = name
-        self._ppas = []
-
-    def createPPA(self, name, description, displayname):
-        for ppa in self._ppas:
-            if ppa.name == name:
-                raise PpaAlreadyExists(name)
-        new_ppa = Ppa(name, self.name, description)
-        self._ppas.append(new_ppa)
-        return True
-
-    def lp_save(self):
-        return True
-
-    @property
-    def ppas(self):
-        return self._ppas
-
-
-class LaunchpadMock:
-    def __init__(self):
-        self.people = {'me': PersonMock('me')}
-
-    def add_person(self, name):
-        print(f"Adding person {name}")
-        self.people[name] = PersonMock(name)
-
-    @property
-    def me(self):
-        return self.people['me']
-
-
-class LpServiceMock:
-    def __init__(self):
-        self.launchpad = LaunchpadMock()
-
-    @property
-    def me(self):
-        return self.launchpad.people['me']
-
-    @property
-    def people(self):
-        return self.launchpad.people
-
-    def get_bug(self, bug_id):
-        class BugMock:
-            @property
-            def title(self):
-                return "Mock bug report"
-
-            @property
-            def description(self):
-                return "Description line 1\n\ndescription line 2"
-
-        return BugMock()
+from tests.helpers import PersonMock, LaunchpadMock, LpServiceMock
 
 
 def test_object():
     """Checks that PpaGroup objects can be instantiated."""
-    ppa_group = PpaGroup(service=LpServiceMock())
-    assert(ppa_group)
+    ppa_group = PpaGroup(service=LpServiceMock(), name=None)
+    assert ppa_group
 
 
 def test_create_ppa():
-    """Checks that PpaGroups can create PPAs"""
+    """Checks that PpaGroups can create PPAs."""
     name = 'test_ppa'
-    ppa_group = PpaGroup(service=LpServiceMock())
+    ppa_group = PpaGroup(service=LpServiceMock(), name='me')
     ppa = ppa_group.create(name)
-    assert(ppa is not None)
-    assert(name in ppa.address)
-    assert(type(ppa.description) is str)
+    assert ppa is not None
+    assert name in ppa.address
+    assert type(ppa.description) is str
 
 
 def test_create_existing_ppa():
-    """Check exception creating an already created PPA"""
+    """Check exception creating an already created PPA."""
     name = 'test_ppa'
-    ppa_group = PpaGroup(service=LpServiceMock())
+    ppa_group = PpaGroup(service=LpServiceMock(), name='me')
     ppa_group.create(name)
     with pytest.raises(PpaAlreadyExists):
         ppa_group.create(name)
 
 
 def test_create_with_description():
-    """Check setting a description for a PPA"""
-    ppa_group = PpaGroup(service=LpServiceMock())
+    """Check setting a description for a PPA."""
+    ppa_group = PpaGroup(service=LpServiceMock(), name='me')
     description = 'PPA Test Description'
     ppa = ppa_group.create('test_ppa_with_description', description)
-    assert(ppa is not None)
-    assert(ppa.description == description)
+    assert ppa is not None
+    assert ppa.description == description
 
 
 def test_create_with_team():
-    """Check creating a PPA for a particular team"""
+    """Check creating a PPA for a particular team."""
     lp = LpServiceMock()
     lp.launchpad.add_person('test_team_name')
     ppa_group = PpaGroup(service=lp, name='test_team_name')
     ppa = ppa_group.create('ppa_test_name')
-    assert(ppa is not None)
-    assert(ppa.address == 'ppa:test_team_name/ppa_test_name')
+    assert ppa is not None
+    assert ppa.address == 'ppa:test_team_name/ppa_test_name'
 
 
 def test_create_for_lpbug():
-    """Check associating a bug # when creating a PPA"""
-    ppa_group = PpaGroup(service=LpServiceMock())
+    """Check associating a bug # when creating a PPA."""
+    ppa_group = PpaGroup(service=LpServiceMock(), name='me')
     lpbug = '1234567'
     ppa = ppa_group.create('lp' + lpbug)
-    assert(ppa is not None)
-    assert(lpbug in ppa.description)
+    assert ppa is not None
+    assert lpbug in ppa.description
 
 
 def test_create_for_merge_proposal():
-    """Check associating a merge proposal when creating a PPA"""
-    ppa_group = PpaGroup(service=LpServiceMock())
+    """Check associating a merge proposal when creating a PPA."""
+    ppa_group = PpaGroup(service=LpServiceMock(), name='me')
     version = '1.2.3-4'
     ppa = ppa_group.create('merge.' + version)
-    assert(ppa is not None)
-    assert(version in ppa.description)
+    assert ppa is not None
+    assert version in ppa.description
 
 
 def test_list_ppas():
-    """Check listing the PPAs for a PPA group"""
+    """Check listing the PPAs for a PPA group."""
     test_ppa_list = ['a', 'b', 'c', 'd']
-    ppa_group = PpaGroup(service=LpServiceMock())
+    ppa_group = PpaGroup(service=LpServiceMock(), name='me')
 
     # Add several ppas
     for ppa in test_ppa_list:
         ppa_group.create(ppa)
 
     ppas = [ppa.name for ppa in list(ppa_group.ppas)]
-    assert(test_ppa_list == ppas)
+    assert test_ppa_list == ppas
diff --git a/tests/test_result.py b/tests/test_result.py
index cbd574c..8c498a4 100644
--- a/tests/test_result.py
+++ b/tests/test_result.py
@@ -17,7 +17,7 @@ import time
 import gzip
 
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 DATA_DIR = os.path.realpath(
     os.path.join(os.path.dirname(__file__), "data"))
 
diff --git a/tests/test_scripts_ppa.py b/tests/test_scripts_ppa.py
index fed190b..7a578f7 100644
--- a/tests/test_scripts_ppa.py
+++ b/tests/test_scripts_ppa.py
@@ -19,7 +19,7 @@ import argparse
 import pytest
 
 SCRIPT_NAME = 'ppa'
-BASE_PATH = os.path.realpath(os.path.join(os.path.dirname(__file__), '..'))
+BASE_PATH = os.path.realpath(os.path.join(os.path.dirname(os.path.realpath(__file__)), '..'))
 sys.path.insert(0, BASE_PATH)
 
 if '.pybuild' in BASE_PATH:
@@ -184,18 +184,6 @@ def test_create_config():
     pass
 
 
-# TODO: Monkeypatch in Lp = MockLp
[e-mail](reason="Unimplemented")
-def test_get_ppa():
-    # TODO: In fake_config, set ppa_name, team_name, and lp
-    # ppa = script.get_ppa(fake_config)
-    # TODO: Verify type(ppa) is Ppa
-    # TODO: Verify that ppa_name is set properly in ppa
-    # TODO: Verify that team_name is set properly in ppa
-    # TODO: Verify that lp is set properly in ppa
-    pass
-
-
 @pytest.mark.xfail(reason="Unimplemented")
 def test_command_create(fake_config):
     assert script.command_create(fake_config) == 0
diff --git a/tests/test_subtest.py b/tests/test_subtest.py
index 04a2dc1..70f37c5 100644
--- a/tests/test_subtest.py
+++ b/tests/test_subtest.py
@@ -16,7 +16,7 @@ import sys
 import pytest
 
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.subtest import Subtest
 
diff --git a/tests/test_trigger.py b/tests/test_trigger.py
index 87092c9..3d8541a 100644
--- a/tests/test_trigger.py
+++ b/tests/test_trigger.py
@@ -16,7 +16,7 @@ import sys
 import pytest
 
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.trigger import Trigger
 
diff --git a/tests/test_version.py b/tests/test_version.py
index fa6066f..63690c3 100644
--- a/tests/test_version.py
+++ b/tests/test_version.py
@@ -10,22 +10,25 @@
 
 import os
 import sys
+
 sys.path.insert(0, os.path.realpath(
-    os.path.join(os.path.dirname(__file__), "..")))
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa._version import __version__, __version_info__
 
 
 def test_version():
-    assert(type(__version__) is str)
-    assert('.' in __version__)
-    assert(__version__[0].isdigit())
-    assert(__version__[-1] != '.')
+    """Checks that the __version__ is specified correctly."""
+    assert type(__version__) is str
+    assert '.' in __version__
+    assert __version__[0].isdigit()
+    assert __version__[-1] != '.'
 
 
 def test_version_info():
-    assert(type(__version_info__) is tuple)
-    assert(len(__version_info__) > 1)
+    """Checks that the __version_info__ is specified correctly."""
+    assert type(__version_info__) is tuple
+    assert len(__version_info__) > 1
     for elem in __version_info__:
-        assert(type(elem) is int)
-        assert(elem >= 0)
+        assert type(elem) is int
+        assert elem >= 0