nagios-charmers team mailing list archive

[Andrea Ieri <andrea.ieri@xxxxxxxxxxxxx>]

Addressed comments

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

Makes sense, I'll update the template.

>  
>  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)

I have added instructions in the README

> +else
> +	@cd src && tox -e functional
> +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

Absolutely... the template is a fast moving target

> +
> +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
> +'''
> +
> +
> +@pytest.yield_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)
> +
> +
> +@pytest.fixture(scope='module')
> +async def controller():
> +    '''Connect to the current controller'''
> +    _controller = Controller()
> +    await _controller.connect_current()
> +    yield _controller
> +    await _controller.disconnect()
> +
> +
> +@pytest.fixture(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)
> +
> +
> +@pytest.fixture()
> +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
> +
> +
> +@pytest.fixture()
> +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
> +
> +
> +@pytest.fixture()
> +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
> +
> +
> +@pytest.fixture
> +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
> +
> +
> +@pytest.fixture
> +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
> +
> +
> +@pytest.fixture
> +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
> +
> +
> +@pytest.fixture
> +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')

+1

> +    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')
> +from hwhealth import hwdiscovery  # noqa: E402
> +from hwhealth import classes      # noqa: E402
> +
> +# 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'
> +
> +
> +###################
> +# Custom fixtures #
> +###################
> +
> +@pytest.fixture(scope='module',
> +                params=SERIES)
> +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

Oh, don't mind me, I'm just throwing names around... I've never actually used jinja :)

> +    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')

oops, of course

> +    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
> +
> +
> +@pytest.fixture(scope='module')
> +async def deployed_unit(deploy_app):
> +    '''Returns the hw-health unit we've deployed'''
> +    return deploy_app.units.pop()

very true, I should just return the first item

> +
> +
> +@pytest.fixture(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'''
> +    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))
> +        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)
> +        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)
> +            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

I would prefer to wait until MP#364040 is reviewed and merged, or at least do the change in a separate commit

> +  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 .

Not really, or rather: yes, but I'd need to tweak two files that are not affected by this oo-rewrite changeset. I think we should address that in a separate MP.

> -


-- 
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.