Merge ~peppepetra/charm-sudo-pair:fix-tests into ~sudo-pair-charmers/charm-sudo-pair:master

[Giuseppe Petralia]

[Alvaro Uria (aluria)]

MP lgtm (only code review, I haven't run make...)
Added a comment inline to possibly remove "make submodules". Thank you.

review: Approve

diff --git a/.gitignore b/.gitignore
index a437a87..eda8bea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,7 +1,33 @@
-__pycache__
-*~
-*.swp
-builds/
-.idea
-.tox
-/repo-info
+# Byte-compiled / optimized / DLL files
+__pycache__/
+*.py[cod]
+*$py.class
+
+# Log files
+*.log
+
+.tox/
+src/.tox/
+.coverage
+
+# vi
+.*.swp
+
+# pycharm
+.idea/
+.unit-state.db
+src/.unit-state.db
+
+# version data
+repo-info
+
+
+# reports
+report/*
+src/report/*
+
+# virtual env
+venv/*
+
+# builds
+builds/*
\ No newline at end of file
diff --git a/Makefile b/Makefile
index 6c2a104..c7506b2 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,9 @@
-PROJECTPATH = $(dir $(realpath $(firstword $(MAKEFILE_LIST))))
-ifndef JUJU_REPOSITORY
-    JUJU_REPOSITORY := $(shell pwd)
-    $(warning Warning JUJU_REPOSITORY was not set, defaulting to $(JUJU_REPOSITORY))
+PROJECTPATH = $(dir $(realpath $(MAKEFILE_LIST)))
+DIRNAME = $(notdir $(PROJECTPATH:%/=%))
+
+ifndef CHARM_BUILD_DIR
+    CHARM_BUILD_DIR := /tmp/$(DIRNAME)-builds
+    $(warning Warning CHARM_BUILD_DIR was not set, defaulting to $(CHARM_BUILD_DIR))
 endif
 
 help:
@@ -9,42 +11,43 @@ help:
 	@echo ""
 	@echo " make help - show this text"
 	@echo " make lint - run flake8"
-	@echo " make test - run the unittests and lint"
-	@echo " make unittest - run the tests defined in the unit subdirectory"
+	@echo " make test - run the functional tests, unittests and lint"
+	@echo " make unittest - run the tests defined in the unittest subdirectory"
 	@echo " make functional - run the tests defined in the functional subdirectory"
 	@echo " make release - build the charm"
 	@echo " make clean - remove unneeded files"
 	@echo ""
 
-submodules:
-	@echo "Cloning submodules"
-	@git submodule update --init --recursive
-
 lint:
 	@echo "Running flake8"
 	@tox -e lint
 
-test: unittest functional lint
+test: lint unittest functional
+
+functional: build
+	@PYTEST_KEEP_MODEL=$(PYTEST_KEEP_MODEL) \
+	    PYTEST_CLOUD_NAME=$(PYTEST_CLOUD_NAME) \
+	    PYTEST_CLOUD_REGION=$(PYTEST_CLOUD_REGION) \
+	    tox -e functional
 
 unittest:
 	@tox -e unit
 
-functional: build
-	@tox -e functional
-
 build:
-	@echo "Building charm to base directory $(JUJU_REPOSITORY)"
-	@-git describe --tags > $(PROJECTPATH)/repo-info
-	@LAYER_PATH=./layers INTERFACE_PATH=./interfaces\
-		JUJU_REPOSITORY=$(JUJU_REPOSITORY) charm build ./ --force
+	@echo "Building charm to base directory $(CHARM_BUILD_DIR)"
+	@-git describe --tags > ./repo-info
+	@CHARM_LAYERS_DIR=./layers CHARM_INTERFACES_DIR=./interfaces TERM=linux\
+		charm build --output-dir $(CHARM_BUILD_DIR) $(PROJECTPATH) --force
 
 release: clean build
-	@echo "Charm is built at $(JUJU_REPOSITORY)/builds"
+	@echo "Charm is built at $(CHARM_BUILD_DIR)/builds"
 
 clean:
 	@echo "Cleaning files"
-	@rm -rf $(PROJECTPATH)/.tox
-	@rm -rf $(PROJECTPATH)/.pytest_cache
+	@find $(PROJECTPATH) -iname __pycache__ -exec rm -r {} +
+	@if [ -d $(CHARM_BUILD_DIR)/builds ] ; then rm -r $(CHARM_BUILD_DIR)/builds ; fi
+	@if [ -d $(PROJECTPATH)/.tox ] ; then rm -r $(PROJECTPATH)/.tox ; fi
+	@if [ -d $(PROJECTPATH)/.pytest_cache ] ; then rm -r $(PROJECTPATH)/.pytest_cache ; fi
 
 # The targets below don't depend on a file
-.PHONY: lint test unittest functional build release clean help submodules
+.PHONY: lint test unittest functional build release clean help
diff --git a/actions/actions.py b/actions/actions.py
index 7dd03d8..ccf3ffc 100755
--- a/actions/actions.py
+++ b/actions/actions.py
@@ -16,7 +16,7 @@
 import os
 import sys
 
-from charmhelpers.core.hookenv import action_set, action_fail
+from charmhelpers.core.hookenv import action_fail, action_set
 
 
 sys.path.append("lib")
@@ -25,7 +25,7 @@ import libsudopair  # NOQA
 
 
 def remove():
-    """Action to remove sudo-pair config and binaries"""
+    """Remove sudo-pair config and binaries."""
     sph = libsudopair.SudoPairHelper()
     sph.deconfigure()
     action_set({"message": "Successfully removed sudo-pair config and binaries"})
diff --git a/lib/libsudopair.py b/lib/libsudopair.py
index 095e9a4..05d1f9d 100644
--- a/lib/libsudopair.py
+++ b/lib/libsudopair.py
@@ -1,9 +1,11 @@
 import grp
 import os
-from charmhelpers.core import host, hookenv, templating
+
+from charmhelpers.core import hookenv, host, templating
 
 
 def check_valid_group(group_name):
+    """Check that a group exists."""
     try:
         grp.getgrnam(group_name)
         return True
@@ -12,12 +14,14 @@ def check_valid_group(group_name):
 
 
 def group_id(group_name):
+    """Check that a group exists."""
     return grp.getgrnam(group_name).gr_gid
 
 
 def group_names_to_group_ids(group_names):
     """
-    From Group Names comma-separated list to Group Ids
+    Return comma-separated list of Group Ids.
+
     :param group_names: i.e. "root,user1,user2"
     :return gids: i.e. "0,1001,1002"
     """
@@ -26,6 +30,7 @@ def group_names_to_group_ids(group_names):
 
 
 def copy_file(source, destination, owner, group, perms):
+    """Copy a file on the unit."""
     if destination is not None:
         target_dir = os.path.dirname(destination)
         if not os.path.exists(target_dir):
@@ -37,7 +42,10 @@ def copy_file(source, destination, owner, group, perms):
 
 
 class SudoPairHelper(object):
+    """Configure sudo-pair."""
+
     def __init__(self):
+        """Retrieve charm config and set defaults."""
         self.charm_config = hookenv.config()
         self.binary_path = '/usr/bin/sudo_approve'
         self.sudo_conf_path = '/etc/sudo.conf'
@@ -58,6 +66,7 @@ class SudoPairHelper(object):
         self.sudo_approve_perms = 0o755
 
     def get_config(self):
+        """Return config as a dict."""
         config = {
             'binary_path': self.binary_path,
             'user_prompt_path': self.user_prompt_path,
@@ -71,41 +80,51 @@ class SudoPairHelper(object):
         return config
 
     def set_charm_config(self, charm_config):
+        """Update configuration."""
         self.charm_config = charm_config
 
     def render_sudo_conf(self):
+        """Render sudo.conf file."""
         return templating.render('sudo.conf.tmpl', self.sudo_conf_path, self.get_config(),
                                  perms=self.sudo_conf_perms, owner=self.owner, group=self.group)
 
     def create_socket_dir(self):
+        """Create socket dir."""
         host.mkdir(self.socket_dir, perms=self.socket_dir_perms, owner=self.owner, group=self.group)
 
     def create_tmpfiles_conf(self):
+        """Create temporary conf file."""
         with open(self.tmpfiles_conf, "w") as f:
             f.write("d {} 0755 - - -\n".format(self.socket_dir))
 
     def install_sudo_pair_so(self):
+        """Install sudo-pair lib."""
         sudo_pair_lib = os.path.join(hookenv.charm_dir(), 'files', 'sudo_pair.so')
         copy_file(sudo_pair_lib, self.sudo_lib_path, self.owner, self.group, self.sudo_pair_so_perms)
 
     def copy_user_prompt(self):
+        """Copy user prompt on the unit."""
         prompt_file = os.path.join(hookenv.charm_dir(), 'files', 'sudo.prompt.user')
         copy_file(prompt_file, self.user_prompt_path, self.owner, self.group, self.prompt_perms)
 
     def copy_pair_prompt(self):
+        """Copy pair prompt on the unit."""
         prompt_file = os.path.join(hookenv.charm_dir(), 'files', 'sudo.prompt.pair')
         copy_file(prompt_file, self.pair_prompt_path, self.owner, self.group, self.prompt_perms)
 
     def copy_sudoers(self):
+        """Copy sudoers file on the unit."""
         sudoers_file = os.path.join(hookenv.charm_dir(), 'files', 'sudoers')
         copy_file(sudoers_file, self.sudoers_path, self.owner, self.group, self.sudoers_perms)
 
     def render_sudo_approve(self):
+        """Render sudo-approve file."""
         hookenv.log("Rendering sudo_approve.tmpl to {}".format(self.binary_path))
         return templating.render('sudo_approve.tmpl', self.binary_path, self.get_config(),
                                  perms=self.sudo_approve_perms, owner=self.owner, group=self.group)
 
     def render_bypass_cmds(self):
+        """Render bypass command file."""
         if self.get_config()['bypass_cmds'] != "" and self.get_config()['bypass_group'] != "":
             hookenv.log("Render bypass cmds to {}".format(self.sudoers_bypass_path))
             return templating.render('91-bypass-sudopair-cmds.tmpl', self.sudoers_bypass_path,
@@ -113,6 +132,7 @@ class SudoPairHelper(object):
         return None
 
     def deconfigure(self):
+        """Remove sudo-pair configuration."""
         paths = [
             self.sudo_conf_path,
             self.sudo_lib_path,
diff --git a/reactive/sudo_pair.py b/reactive/sudo_pair.py
index be557f8..974dd66 100644
--- a/reactive/sudo_pair.py
+++ b/reactive/sudo_pair.py
@@ -1,6 +1,6 @@
-from charms.reactive import when, when_not, set_state, remove_state, hook
 from charmhelpers.core import hookenv
 
+from charms.reactive import hook, remove_state, set_state, when, when_not
 
 from libsudopair import SudoPairHelper
 
diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
index 7279748..aa42a09 100644
--- a/tests/functional/conftest.py
+++ b/tests/functional/conftest.py
@@ -2,21 +2,23 @@
 
 import asyncio
 import json
-import juju
 import os
-import pytest
 import uuid
+
+import juju
 from juju.controller import Controller
 from juju.errors import JujuError
 from juju.model import Model
 
+import pytest
+
+
 STAT_FILE = "python3 -c \"import json; import os; s=os.stat('%s'); print(json.dumps({'uid': s.st_uid, 'gid': s.st_gid, 'mode': oct(s.st_mode), 'size': s.st_size}))\""  # noqa: E501
 
 
 @pytest.yield_fixture(scope='module')
 def event_loop(request):
-    '''Override the default pytest event loop to allow for broaded scoped
-    fixtures'''
+    """Override the default pytest event loop to allow for broaded scopedv fixtures."""
     loop = asyncio.get_event_loop_policy().new_event_loop()
     asyncio.set_event_loop(loop)
     loop.set_debug(True)
@@ -27,7 +29,7 @@ def event_loop(request):
 
 @pytest.fixture(scope='module')
 async def controller():
-    '''Connect to the current controller'''
+    """Connect to the current controller."""
     controller = Controller()
     await controller.connect_current()
     yield controller
@@ -36,7 +38,7 @@ async def controller():
 
 @pytest.fixture(scope='module')
 async def model(controller):
-    '''This model lives only for the duration of the test'''
+    """Create a model that lives only for the duration of the test."""
     model_name = "functest-{}".format(uuid.uuid4())
     model = await controller.add_model(model_name)
     yield model
@@ -50,7 +52,7 @@ async def model(controller):
 
 @pytest.fixture(scope='module')
 async def current_model():
-    '''Returns the current model, does not create or destroy it'''
+    """Return the current model, does not create or destroy it."""
     model = Model()
     await model.connect_current()
     yield model
@@ -59,7 +61,7 @@ async def current_model():
 
 @pytest.fixture
 async def get_app(model):
-    '''Returns the application requested'''
+    """Return the application requested."""
     async def _get_app(name):
         try:
             return model.applications[name]
@@ -70,7 +72,7 @@ async def get_app(model):
 
 @pytest.fixture
 async def get_unit(model):
-    '''Returns the requested <app_name>/<unit_number> unit'''
+    """Return the requested <app_name>/<unit_number> unit."""
     async def _get_unit(name):
         try:
             (app_name, unit_number) = name.split('/')
@@ -82,7 +84,7 @@ async def get_unit(model):
 
 @pytest.fixture
 async def get_entity(model, get_unit, get_app):
-    '''Returns a unit or an application'''
+    """Return a unit or an application."""
     async def _get_entity(name):
         try:
             return await get_unit(name)
@@ -96,12 +98,12 @@ async def get_entity(model, get_unit, get_app):
 
 @pytest.fixture
 async def run_command(get_unit):
-    '''
-    Runs a command on a unit.
+    """
+    Run a command on a unit.
 
     :param cmd: Command to be run
     :param target: Unit object or unit name string
-    '''
+    """
     async def _run_command(cmd, target):
         unit = (
             target
@@ -115,12 +117,12 @@ async def run_command(get_unit):
 
 @pytest.fixture
 async def file_stat(run_command):
-    '''
-    Runs stat on a file
+    """
+    Run stat on a file.
 
     :param path: File path
     :param target: Unit object or unit name string
-    '''
+    """
     async def _file_stat(path, target):
         cmd = STAT_FILE % path
         results = await run_command(cmd, target)
@@ -130,12 +132,12 @@ async def file_stat(run_command):
 
 @pytest.fixture
 async def file_contents(run_command):
-    '''
-    Returns the contents of a file
+    """
+    Return the contents of a file.
 
     :param path: File path
     :param target: Unit object or unit name string
-    '''
+    """
     async def _file_contents(path, target):
         cmd = 'cat {}'.format(path)
         results = await run_command(cmd, target)
@@ -145,7 +147,7 @@ async def file_contents(run_command):
 
 @pytest.fixture
 async def reconfigure_app(get_app, model):
-    '''Applies a different config to the requested app'''
+    """Apply a different config to the requested app."""
     async def _reconfigure_app(cfg, target):
         application = (
             target
@@ -160,7 +162,7 @@ async def reconfigure_app(get_app, model):
 
 @pytest.fixture
 async def create_group(run_command):
-    '''Creates the UNIX group specified'''
+    """Create the UNIX group specified."""
     async def _create_group(group_name, target):
         cmd = "sudo groupadd %s" % group_name
         await run_command(cmd, target)
diff --git a/tests/functional/requirements.txt b/tests/functional/requirements.txt
index b9815c2..1da5a06 100644
--- a/tests/functional/requirements.txt
+++ b/tests/functional/requirements.txt
@@ -2,5 +2,6 @@ juju
 requests
 pytest
 pytest-asyncio
+pytest-cov
 mock
 flake8
diff --git a/tests/functional/test_deploy.py b/tests/functional/test_deploy.py
index 5c8e690..cdb5ae4 100644
--- a/tests/functional/test_deploy.py
+++ b/tests/functional/test_deploy.py
@@ -1,38 +1,67 @@
 #!/usr/bin/python3.6
-import asyncio
+
 import os
+
 import pytest
 
 pytestmark = pytest.mark.asyncio
 
+charm_build_dir = os.getenv('CHARM_BUILD_DIR', '..').rstrip('/')
+
+
+sources = [('local', '{}/builds/sudo-pair'.format(charm_build_dir))]
+
+series = ['xenial',
+          'bionic',
+          ]
 
-def get_series():
-    series = os.getenv('test_series', 'xenial bionic').strip()
-    return series.split()
 
 ############
 # FIXTURES #
 ############
 
 
-# This fixture shouldn't really be in conftest.py since it's specific to this
-# charm
[e-mail](scope='module',
-                params=get_series())
-async def deploy_app(request, model):
-    '''Deploys the sudo_pair app as a subordinate of ubuntu'''
-    release = request.param
[e-mail](params=series)
+def series(request):
+    """Return ubuntu version (i.e. xenial) in use in the test."""
+    return request.param
+
+
[e-mail](params=sources, ids=[s[0] for s in sources])
+def source(request):
+    """Return source of the charm under test (i.e. local, cs)."""
+    return request.param
+
+
[e-mail]
+async def app(model, series, source):
+    """Return application of the charm under test."""
+    app_name = 'sudo-pair-{}'.format(series)
+    return await model._wait_for_new('application', app_name)
+
+
[e-mail]
+async def unit(app):
+    """Return the sudo_pair unit we've deployed."""
+    return app.units[0]
+
+
+#########
+# TESTS #
+#########
 
+async def test_deploy_app(model, series, source):
+    """Deploy the sudo_pair app as a subordinate of ubuntu."""
     await model.deploy(
         'ubuntu',
-        application_name='ubuntu-' + release,
-        series=release,
+        application_name='ubuntu-' + series,
+        series=series,
         channel='stable'
     )
     sudo_pair_app = await model.deploy(
-        '{}/builds/sudo-pair'.format(os.getenv('JUJU_REPOSITORY')),
-        application_name='sudo-pair-' + release,
-        series=release,
+        source[1],
+        application_name='sudo-pair-' + series,
+        series=series,
         num_units=0,
         config={
             'bypass_cmds': '/bin/ls',
@@ -41,27 +70,17 @@ async def deploy_app(request, model):
         }
     )
     await model.add_relation(
-        'ubuntu-{}:juju-info'.format(release),
-        'sudo-pair-{}:juju-info'.format(release))
+        'ubuntu-{}:juju-info'.format(series),
+        'sudo-pair-{}:juju-info'.format(series))
 
     await model.block_until(lambda: sudo_pair_app.status == 'active')
-    yield sudo_pair_app
     # no need to cleanup since the model will be be torn down at the end of the
     # testing
 
 
[e-mail](scope='module')
-async def unit(deploy_app):
-    '''Returns the sudo_pair unit we've deployed'''
-    return deploy_app.units.pop()
-
-#########
-# TESTS #
-#########
-
-
-async def test_deploy(deploy_app):
-    assert deploy_app.status == 'active'
+async def test_status(app):
+    """Check that the app is in active state."""
+    assert app.status == 'active'
 
 
 @pytest.mark.parametrize("path,expected_stat", [
@@ -86,6 +105,7 @@ async def test_deploy(deploy_app):
         'uid': 0,
         'mode': '0o40644'})])
 async def test_stats(path, expected_stat, unit, file_stat):
+    """Check that created files have the correct permissions."""
     test_stat = await file_stat(path, unit)
     assert test_stat['size'] > 0
     assert test_stat['gid'] == expected_stat['gid']
@@ -94,11 +114,13 @@ async def test_stats(path, expected_stat, unit, file_stat):
 
 
 async def test_sudoers(file_contents, unit):
+    """Check the content of sudoers file."""
     sudoers_content = await file_contents("/etc/sudoers", unit)
     assert 'Defaults	log_output' in sudoers_content
 
 
 async def test_sudoers_bypass_conf(file_contents, unit):
+    """Check the content of sudoers bypass command file."""
     path = "/etc/sudoers.d/91-bypass-sudopair-cmds"
     sudoers_bypass_content = await file_contents(path=path,
                                                  target=unit)
@@ -106,41 +128,30 @@ async def test_sudoers_bypass_conf(file_contents, unit):
     assert content in sudoers_bypass_content
 
 
-async def test_reconfigure(reconfigure_app, file_contents, unit, deploy_app):
-    '''Change a charm config parameter and verify that it has been propagated to
-    the unit'''
+async def test_reconfigure(reconfigure_app, file_contents, unit, app):
+    """Change a charm config parameter and verify that it has been propagated to the unit."""
     sudo_approve_path = '/usr/bin/sudo_approve'
     await reconfigure_app(cfg={'auto_approve': 'false'},
-                          target=deploy_app)
+                          target=app)
     sudo_approve_content = await file_contents(path=sudo_approve_path,
                                                target=unit)
     new_content = 'echo "You can\'t approve your own session."'
     assert new_content in sudo_approve_content
 
 
-async def test_remove_relation(deploy_app, model, run_command):
-    series = deploy_app.units[0].data['series']
+async def test_remove_relation(app, model, run_command):
+    """Check that the relation is removed."""
+    series = app.units[0].data['series']
     app_name = 'sudo-pair-{}'.format(series)
     principalname = 'ubuntu-{}'.format(series)
-    await deploy_app.remove_relation(
+    await app.remove_relation(
         '{}:juju-info'.format(app_name),
         '{}:juju-info'.format(principalname))
-    await model.block_until(lambda: not deploy_app.relations)
+    await model.block_until(lambda: not app.relations)
     principal = model.applications[principalname].units[0]
     res = await run_command('test -f /etc/sudo.conf || echo gone', target=principal)
     assert res['Stdout'].strip() == 'gone'
     await model.add_relation(
         '{}:juju-info'.format(principalname),
         '{}:juju-info'.format(app_name))
-    await model.block_until(lambda: deploy_app.relations)
-
-
-async def test_remove_unit(deploy_app, model, run_command):
-    series = deploy_app.units[0].data['series']
-    app_name = 'sudo-pair-{}'.format(series)
-    await deploy_app.destroy()
-    while app_name in model.applications:
-        await asyncio.sleep(2)
-    principal = model.applications['ubuntu-{}'.format(series)].units[0]
-    res = await run_command('test -f /etc/sudo.conf || echo gone', target=principal)
-    assert res['Stdout'].strip() == 'gone'
+    await model.block_until(lambda: app.relations)
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index 0e2c034..c0800ea 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -1,11 +1,12 @@
 #!/usr/bin/python3
 
-import pytest
-import pwd
 import grp
 import os
+import pwd
 from pathlib import Path
 
+import pytest
+
 
 @pytest.fixture
 def mock_hookenv_config(monkeypatch):
diff --git a/tests/unit/requirements.txt b/tests/unit/requirements.txt
index 081ed97..04dbf13 100644
--- a/tests/unit/requirements.txt
+++ b/tests/unit/requirements.txt
@@ -2,3 +2,4 @@ charmhelpers
 charms.reactive
 pytest
 mock
+pytest-cov
\ No newline at end of file
diff --git a/tests/unit/test_libsudopair.py b/tests/unit/test_libsudopair.py
index fea4985..3a598af 100644
--- a/tests/unit/test_libsudopair.py
+++ b/tests/unit/test_libsudopair.py
@@ -1,6 +1,6 @@
-import os
-import grp
 import filecmp
+import grp
+import os
 
 from libsudopair import (
     check_valid_group,
@@ -18,14 +18,18 @@ def test_group_id():
 
 
 class TestSudoPairHelper():
+    """Module to test SudoPairHelper lib."""
+
     def test_pytest(self):
+        """Assert testing is carryied using pytest."""
         assert True
 
     def test_sph(self, sph):
-        ''' See if the ph fixture works to load charm configs '''
+        """See if the sph fixture works to load charm configs."""
         assert isinstance(sph.charm_config, dict)
 
     def test_get_config(self, sph):
+        """Check if config contains all the required entries."""
         default_keywords = [
             'binary_path',
             'user_prompt_path',
@@ -39,6 +43,7 @@ class TestSudoPairHelper():
             assert option in config
 
     def test_set_charm_config(self, sph):
+        """Set new config."""
         charm_config = {
             'groups_enforced': 'root',
             'groups_exempted': '',
@@ -54,6 +59,7 @@ class TestSudoPairHelper():
             assert sph.get_config()[option] == charm_config[option]
 
     def test_render_sudo_conf(self, sph, tmpdir):
+        """Check that sudo.conf is rendered correctly."""
         # Default config
         content = sph.render_sudo_conf()
         expected_content = 'Plugin sudo_pair sudo_pair.so binary_path={} ' \
@@ -107,7 +113,8 @@ class TestSudoPairHelper():
         content = sph.render_sudo_conf()
         assert expected_content in content
 
-    def test_render_bypass_cmds(self, sph, tmpdir):
+    def test_render_bypass_cmds(self, sph):
+        """Check that sudoers file is rendered correctly."""
         # Root bypass /bin/ls
         expected_content = '%root ALL = (ALL) NOLOG_OUTPUT: /bin/ls'
         charm_config = {
@@ -122,6 +129,7 @@ class TestSudoPairHelper():
         assert expected_content in content
 
     def test_render_sudo_approve(self, sph, tmpdir):
+        """Check that sudo_approve file is rendered correctly."""
         # Auto Approve true
         expected_content = 'echo ${log_line} >> /var/log/sudo_pair.log'
         socket_dir = tmpdir.join('/var/run/sudo_pair')
@@ -144,10 +152,12 @@ class TestSudoPairHelper():
         assert expected_content in content
 
     def test_create_socket_dir(self, sph, tmpdir):
+        """Check that sudo_pair socket dir exists."""
         sph.create_socket_dir()
         assert os.path.exists(tmpdir.join('/var/run/sudo_pair'))
 
     def test_create_tmpfiles_conf(self, sph, tmpdir):
+        """Check that sudo pair temporary conf is rendered correctly."""
         sph.create_tmpfiles_conf()
         expected_content = 'd {} 0755 - - -\n'.format(sph.socket_dir)
         with open(tmpdir.join('/usr/lib/tmpfiles.d/sudo_pair.conf')) as f:
@@ -155,17 +165,21 @@ class TestSudoPairHelper():
         assert expected_content in content
 
     def test_install_sudo_pair_so(self, sph, tmpdir):
+        """Check that sudo system lib exists."""
         sph.install_sudo_pair_so()
         assert filecmp.cmp('./files/sudo_pair.so', tmpdir.join('/usr/lib/sudo/sudo_pair.so'))
 
     def test_copy_user_prompt(self, sph, tmpdir):
+        """Check that user prompt exists."""
         sph.copy_user_prompt()
         assert filecmp.cmp('./files/sudo.prompt.user', tmpdir.join('/etc/sudo_pair.prompt.user'))
 
     def test_copy_pair_prompt(self, sph, tmpdir):
+        """Check that pair prompt exists."""
         sph.copy_pair_prompt()
         assert filecmp.cmp('./files/sudo.prompt.pair', tmpdir.join('/etc/sudo_pair.prompt.pair'))
 
     def test_copy_sudoers(self, sph, tmpdir):
+        """Check that sudoers file exists."""
         sph.copy_sudoers()
         assert filecmp.cmp('./files/sudoers', tmpdir.join('/etc/sudoers'))
diff --git a/tox.ini b/tox.ini
index 562d0ba..fc364de 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,6 @@
 [tox]
 skipsdist=True
-envlist = unit, functional, func_xenial
+envlist = unit, functional
 skip_missing_interpreters = True
 
 [testenv]
@@ -9,38 +9,43 @@ setenv =
   PYTHONPATH = .
 
 [testenv:unit]
-commands = pytest -v --ignore {toxinidir}/tests/amulet --ignore {toxinidir}/tests/functional
+commands = pytest -v --ignore {toxinidir}/tests/functional \
+	   --cov=lib \
+	   --cov=reactive \
+	   --cov=actions \
+	   --cov-report=term \
+	   --cov-report=annotate:report/annotated \
+	   --cov-report=html:report/html
 deps = -r{toxinidir}/tests/unit/requirements.txt
+
 setenv = PYTHONPATH={toxinidir}/lib
 
 [testenv:functional]
 passenv =
   HOME
-  JUJU_REPOSITORY
+  CHARM_BUILD_DIR
   PATH
-commands = pytest -v --ignore {toxinidir}/tests/unit --ignore {toxinidir}/tests/amulet
+  PYTEST_KEEP_MODEL
+  PYTEST_CLOUD_NAME
+  PYTEST_CLOUD_REGION
+commands = pytest -v --ignore {toxinidir}/tests/unit
 deps = -r{toxinidir}/tests/functional/requirements.txt
 
 
-[testenv:func_xenial]
-passenv =
-  HOME
-  JUJU_REPOSITORY
-  PATH
-  test_preserve_model
-setenv = test_series=xenial
-commands = pytest -v --ignore {toxinidir}/tests/unit --ignore {toxinidir}/tests/amulet 
-deps = -r{toxinidir}/tests/functional/requirements.txt
-
 [testenv:lint]
 commands = flake8
-deps = flake8
+deps =
+    flake8
+    flake8-docstrings
+    flake8-import-order
+    pep8-naming
+    flake8-colors
 
 [flake8]
-max-line-length = 120
-max-complexity=10
-ignore = W503
+ignore = D100,D103  # Missing docstring in public module/function
 exclude =
     .git,
     __pycache__,
     .tox,
+max-line-length = 120
+max-complexity = 10