Review: Approve
Seems fine. Some inline comments and minor issues that should be fixed before
landing.
Diff comments:
> diff --git a/Makefile b/Makefile
> index 20a6f9b..a5668bc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -9,26 +9,29 @@ help:
> @echo " make help - show this text"
> @echo " make lint - use pre-commit to ensure consistent layout"
> @echo " make test - run the functional test, unittests and lint"
> - @echo " make unittest - run the tests defined in the unittest "\
> - "subdirectory"
> - @echo " make functionaltest - run the tests defined in the "\
> - "functional subdirectory"
> + @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 ""
> - @echo " the following targets are meant to be used by the Makefile"
> - @echo " make requirements - installs the requirements"
>
> lint:
> @echo "Running flake8"
> cd src && tox -e pep8
>
>
> -test: unittest lint
> +test: unittest functional lint
Personally, I put lint first, followed by unittest, because they run fastest. I
understand that this comes from the template, so might be worth fixing there if
you agree.
>
> unittest:
> @cd src && tox -e unit
>
> +functional: build
> +ifeq ("$(wildcard $(JUJU_REPOSITORY)/tools.zip)","")
> + $(error ERROR: You must save the tools resource in
> ${JUJU_REPOSITORY}/tools.zip before running functional tests)
Is there a download link for tools.zip, or a wiki page or doc on how to
assemble it? The failure message needs to help a new charm contributor resolve
the problem, even if it just tells the to look in the README.
I like how you did this in Makefile syntax rather than my usual approach of
putting the if/then in the rule shell code.
> +else
> + @cd src && tox -e functional
The latest release of the template moves things back out of src/ (and has a
more complex command here). No need to change though, as updating things to
match the latest version of that particular moving target should wait until
later unless it solves some other problem.
> +endif
> +
> build:
> @echo "Building charm to base directory $(JUJU_REPOSITORY)"
> @git show -s --format="commit-sha-1: %H%ncommit-short: %h" >
> ./src/repo-info
> diff --git a/src/tests/functional/conftest.py
> b/src/tests/functional/conftest.py
> new file mode 100644
> index 0000000..ea22194
> --- /dev/null
> +++ b/src/tests/functional/conftest.py
> @@ -0,0 +1,171 @@
> +#!/usr/bin/python3
> +'''
> +Reusable pytest fixtures for functional testing
This is all from the template. Not a problem for this branch, but we need to
work out a better way of distributing this shared code (such as a pypi package,
embedded via wheelhouse.txt), to avoid remaking the past mistake of embedded
charm-helpers.
> +
> +Environment variables
> +---------------------
> +
> +test_preserve_model:
> +if set, the testing model won't be torn down at the end of the testing
> session
> +'''
> +
> +import asyncio
> +import json
> +import os
> +import uuid
> +import pytest
> +import juju
> +from juju.controller import Controller
> +from juju.errors import JujuError
> +
> +STAT_CMD = '''python3 - <<EOF
> +import json
> +import os
> +
> +s = os.stat('%s')
> +stat_hash = {
> + 'uid': s.st_uid,
> + 'gid': s.st_gid,
> + 'mode': oct(s.st_mode),
> + 'size': s.st_size
> +}
> +stat_json = json.dumps(stat_hash)
> +print(stat_json)
> +
> +EOF
> +'''
> +
> +
> [email protected]_fixture(scope='module')
> +def event_loop():
> + '''Override the default pytest event loop to allow for fixtures using a
> + broader scope'''
> + loop = asyncio.get_event_loop_policy().new_event_loop()
> + asyncio.set_event_loop(loop)
> + loop.set_debug(True)
> + yield loop
> + loop.close()
> + asyncio.set_event_loop(None)
> +
> +
> [email protected](scope='module')
> +async def controller():
> + '''Connect to the current controller'''
> + _controller = Controller()
> + await _controller.connect_current()
> + yield _controller
> + await _controller.disconnect()
> +
> +
> [email protected](scope='module')
> +async def model(controller): # pylint: disable=redefined-outer-name
> + '''This model lives only for the duration of the test'''
> + model_name = "functest-{}".format(uuid.uuid4())
> + _model = await controller.add_model(model_name)
> + yield _model
> + await _model.disconnect()
> + if not os.getenv('test_preserve_model'):
> + await controller.destroy_model(model_name)
> + while model_name in await controller.list_models():
> + await asyncio.sleep(1)
> +
> +
> [email protected]()
> +async def get_app(model): # pylint: disable=redefined-outer-name
> + '''Returns the application requested'''
> + async def _get_app(name):
> + try:
> + return model.applications[name]
> + except KeyError:
> + raise JujuError("Cannot find application {}".format(name))
> + return _get_app
> +
> +
> [email protected]()
> +async def get_unit(model): # pylint: disable=redefined-outer-name
> + '''Returns the requested <app_name>/<unit_number> unit'''
> + async def _get_unit(name):
> + try:
> + (app_name, unit_number) = name.split('/')
> + return model.applications[app_name].units[unit_number]
> + except (KeyError, ValueError):
> + raise JujuError("Cannot find unit {}".format(name))
> + return _get_unit
> +
> +
> [email protected]()
> +async def get_entity(get_unit, get_app): # pylint:
> disable=redefined-outer-name
> + '''Returns a unit or an application'''
> + async def _get_entity(name):
> + try:
> + return await get_unit(name)
> + except JujuError:
> + try:
> + return await get_app(name)
> + except JujuError:
> + raise JujuError("Cannot find entity {}".format(name))
> + return _get_entity
> +
> +
> [email protected]
> +async def run_command(get_unit): # pylint: disable=redefined-outer-name
> + '''
> + Runs 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
> + if isinstance(target, juju.unit.Unit)
> + else await get_unit(target)
> + )
> + action = await unit.run(cmd)
> + return action.results
> + return _run_command
> +
> +
> [email protected]
> +async def file_stat(run_command): # pylint: disable=redefined-outer-name
> + '''
> + Runs stat on a file
> +
> + :param path: File path
> + :param target: Unit object or unit name string
> + '''
> + async def _file_stat(path, target):
> + cmd = STAT_CMD % path
> + results = await run_command(cmd, target)
> + return json.loads(results['Stdout'])
> + return _file_stat
> +
> +
> [email protected]
> +async def file_contents(run_command): # pylint: disable=redefined-outer-name
> + '''
> + Returns 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)
> + return results['Stdout']
> + return _file_contents
> +
> +
> [email protected]
> +async def reconfigure_app(get_app, model): # pylint:
> disable=redefined-outer-name
> + '''Applies a different config to the requested app'''
> + async def _reconfigure_app(cfg, target):
> + application = (
> + target
> + if isinstance(target, juju.application.Application)
> + else await get_app(target)
> + )
> + await application.set_config(cfg)
> + await application.get_config()
> + await model.block_until(lambda: application.status == 'active')
> + return _reconfigure_app
> diff --git a/src/tests/functional/test_hwhealth.py
> b/src/tests/functional/test_hwhealth.py
> new file mode 100644
> index 0000000..bf905c1
> --- /dev/null
> +++ b/src/tests/functional/test_hwhealth.py
> @@ -0,0 +1,183 @@
> +import os
> +import pytest
> +import sys
> +import subprocess
> +import asyncio
> +sys.path.append('lib')
Needs a blank line before and after the sys.path.append.
> +from hwhealth import hwdiscovery # noqa: E402
> +from hwhealth import classes # noqa: E402
> +
Two blank lines after the imports per pep8
> +# Treat all tests as coroutines
> +pytestmark = pytest.mark.asyncio
> +SERIES = ['xenial', 'bionic']
> +juju_repository = os.getenv('JUJU_REPOSITORY', '.').rstrip('/')
> +nrpecfg_dir = '/etc/nagios/nrpe.d'
juju_repository and nrpecfg_dir are module level constants and per pep8 should
be ALL_CAPS.
> +
> +
> +###################
> +# Custom fixtures #
> +###################
> +
> [email protected](scope='module',
> + params=SERIES)
No need for this to be split over two lines; it would read better as a single
line.
> +async def deploy_app(request, model):
> + '''Deploys the hw-health charm as a subordinate of ubuntu'''
> + # TODO: this might look nicer if we deployed a bundle instead. It could
> be
> + # a jinja template to handle the parametrization
Or just manipulate the machine readable YAML file with Python, rather than
continue the terrible habit of using HTML or text templating to generate
structured YAML/JSON :-P
> + release = request.param
> + channel = 'stable'
> + hw_health_app_name = 'hw-health-' + release
> +
> + for principal_app in ['ubuntu', 'nagios']:
> + await model.deploy(
> + principal_app,
> + application_name='{}-{}'.format(principal_app, release),
> + series=release,
> + channel=channel,
> + )
> + nrpe_app = await model.deploy(
> + 'nrpe',
> + application_name='nrpe-' + release,
> + series=release,
> + num_units=0,
> + channel=channel,
> + )
> + await nrpe_app.add_relation(
> + 'general-info',
> + 'ubuntu-{}:juju-info'.format(release)
> + )
> + await nrpe_app.add_relation(
> + 'monitors',
> + 'nagios-{}:monitors'.format(release)
> + )
> +
> + # Attaching resources is not implemented yet in libjuju
> + # see https://github.com/juju/python-libjuju/issues/294
> + tools_res_path = os.path.join(os.getenv('JUJU_REPOSITORY'), 'tools.zip')
You setup the JUJU_REPOSITORY variable at the top of the module, so you
probably should actually make use of it here, and again half a dozen lines down.
> + subprocess.check_call([
> + 'juju',
> + 'deploy',
> + '-m',
> + model.info.name,
> + os.path.join(os.getenv('JUJU_REPOSITORY'), 'builds', 'hw-health'),
> + hw_health_app_name,
> + '--resource',
> + 'tools={}'.format(tools_res_path),
> + ])
> + # This is pretty horrible, but we can't deploy via libjuju
> + while True:
> + try:
> + hw_health_app = model.applications[hw_health_app_name]
> + break
> + except KeyError:
> + await asyncio.sleep(5)
> +
> + await hw_health_app.add_relation(
> + 'general-info',
> + 'ubuntu-{}:juju-info'.format(release)
> + )
> + await hw_health_app.add_relation(
> + 'nrpe-external-master',
> + nrpe_app.name + ':nrpe-external-master'
> + )
> + # The app will initially be in blocked state because it's running in a
> + # container
> + await model.block_until(lambda: hw_health_app.status == 'blocked')
> + yield hw_health_app
> +
> +
> [email protected](scope='module')
> +async def deployed_unit(deploy_app):
> + '''Returns the hw-health unit we've deployed'''
> + return deploy_app.units.pop()
I don't understand when these fixtures get invoked. Are you sure .pop() is the
right thing to do here, mutating deploy_app.units list?
> +
> +
> [email protected](scope='function')
> +async def tools(monkeypatch):
> + '''All tool classes know which files should be installed and how, so we
> can
> + use them to read the expected stat results. Monkeypatching is however
> + required as the classes code is not expected to be run outside of a
> + deployed charm'''
This isn't a well formed docstring. It reads more like a comment to me, so
maybe it should be one. Docstring is just something like 'Return a tool class
for use by tests'.
> + with monkeypatch.context() as m:
> + m.setattr('charmhelpers.core.hookenv.charm_dir',
> + lambda: os.getenv('JUJU_REPOSITORY'))
> + m.setattr('charmhelpers.core.hookenv.config',
> + lambda x=None: dict())
> +
> m.setattr('charmhelpers.contrib.charmsupport.nrpe.get_nagios_hostname',
> + lambda: 'pytest')
> + return hwdiscovery.get_tools('test')
> +
> +#########
> +# Tests #
> +#########
> +
> +
> +async def test_cannot_run_in_container(deploy_app):
> + assert deploy_app.status == 'blocked'
> +
> +
> +async def test_forced_deploy(deploy_app, model):
> + await deploy_app.set_config({'manufacturer': 'test'})
> + await model.block_until(lambda: deploy_app.status == 'active')
> + assert deploy_app.status == 'active'
> +
> +
> +async def test_stats(tools, deployed_unit, file_stat):
> + # This should really be a parametrized test, but fixtures cannot be used
> as
> + # params value as if they were iterators
> + # It should also check for other installed files and differentiate
> between
> + # tool types (e.g. IpmiTool does not use a vendor binary)
> + for tool in tools:
> + # Have we rendered the nrpe check cfg?
> + nrpecfg_path = os.path.join(nrpecfg_dir,
> + 'check_{}.cfg'.format(tool._shortname))
This would read better on a single line.
> + print('Checking {}'.format(nrpecfg_path))
> + test_stat = await file_stat(nrpecfg_path, deployed_unit)
> + assert test_stat['size'] > 0
> +
> + # Have we installed the nrpe check script?
> + nrpescript_path = os.path.join(tool.NRPE_PLUGINS_DIR,
> + tool._nrpe_script)
Single line
> + print('Checking {}'.format(nrpescript_path))
> + test_stat = await file_stat(nrpescript_path, deployed_unit)
> + assert test_stat['size'] > 0
> + assert test_stat['gid'] == tool.NRPE_PLUGINS_GID
> + assert test_stat['uid'] == tool.NRPE_PLUGINS_UID
> + assert test_stat['mode'] == oct(tool.NRPE_PLUGINS_MODE)
> + if isinstance(tool, classes.IpmiTool):
> + # Have we added sudo rights for running freeipmi commands?
> + sudoer_path = os.path.join(tool.SUDOERS_DIR, tool._sudoer_file)
> + print('Checking {}'.format(sudoer_path))
> + test_stat = await file_stat(sudoer_path, deployed_unit)
> + assert test_stat['size'] > 0
> + assert test_stat['gid'] == tool.SUDOERS_GID
> + assert test_stat['uid'] == tool.SUDOERS_UID
> + assert test_stat['mode'] == oct(tool.SUDOERS_MODE)
> + elif isinstance(tool, classes.VendorTool):
> + # Have we installed the cronjob script helper?
> + cronjob_script_path = os.path.join(tool.NRPE_PLUGINS_DIR,
> + tool._cronjob_script)
Single line.
> + print('Checking {}'.format(cronjob_script_path))
> + test_stat = await file_stat(cronjob_script_path, deployed_unit)
> + assert test_stat['size'] > 0
> + assert test_stat['gid'] == tool.CRONJOB_SCRIPT_GID
> + assert test_stat['uid'] == tool.CRONJOB_SCRIPT_UID
> + assert test_stat['mode'] == oct(tool.CRONJOB_SCRIPT_MODE)
> +
> + # Have we installed the cronjob itself?
> + cronjob_path = os.path.join(tool.CROND_DIR, tool._shortname)
> + print('Checking {}'.format(cronjob_path))
> + test_stat = await file_stat(cronjob_path, deployed_unit)
> + assert test_stat['size'] > 0
> +
> + # Have we installed the vendor binary?
> + if isinstance(tool, classes.MdadmTool):
> + bin_path = os.path.join('/sbin', tool._shortname)
> + else:
> + bin_path = os.path.join(tool.TOOLS_DIR, tool._shortname)
> + print('Checking {}'.format(bin_path))
> + test_stat = await file_stat(bin_path, deployed_unit)
> + assert test_stat['size'] > 0
> + assert test_stat['gid'] == tool.TOOLS_GID
> + assert test_stat['uid'] == tool.TOOLS_UID
> + assert test_stat['mode'] == oct(tool.TOOLS_MODE)
> diff --git a/src/tox.ini b/src/tox.ini
> index 04b6de8..77597a0 100644
> --- a/src/tox.ini
> +++ b/src/tox.ini
> @@ -12,9 +12,17 @@ commands =
> {toxworkdir}/../tests/download_nagios_plugin3.py
> nosetests tests/unit
>
> +[testenv:functional]
> +passenv =
> + HOME
> + JUJU_REPOSITORY
> + PATH
> +commands = pytest -v --ignore {toxinidir}/tests/unit
> +deps = -r{toxinidir}/tests/functional/requirements.txt
> + -r{toxinidir}/requirements.txt
> +
> [testenv:pep8]
> basepython = python3
> deps =
> flake8
> commands = flake8 {posargs} --max-complexity=20 --max-line-length=120 .
Can you ratchet down the complexity yet? 20 is pretty high, with 10 being what
I see most people targetting.
> -
--
https://code.launchpad.net/~aieri/hw-health-charm/+git/hw-health-charm/+merge/364758
Your team Nagios Charm developers is subscribed to branch
hw-health-charm:oo-rewrite-integration.
--
Mailing list: https://launchpad.net/~nagios-charmers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~nagios-charmers
More help : https://help.launchpad.net/ListHelp
