Merge pull request #4913 from sosiouxme/20170720-refactor-check-results

openshift_checks: refactor check results

8 files changed, 307 insertions, 250 deletions

diff --git a/roles/openshift_health_checker/test/action_plugin_test.py b/roles/openshift_health_checker/test/action_plugin_test.py
index 2d068be3d..f5161d6f5 100644
--- a/roles/openshift_health_checker/test/action_plugin_test.py
+++ b/roles/openshift_health_checker/test/action_plugin_test.py
@@ -6,7 +6,7 @@ from openshift_health_check import ActionModule, resolve_checks
 from openshift_checks import OpenShiftCheckException
 
 
-def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, run_exception=None):
+def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, run_exception=None, changed=False):
     """Returns a new class that is compatible with OpenShiftCheck for testing."""
 
     _name, _tags = name, tags
@@ -14,6 +14,7 @@ def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, ru
     class FakeCheck(object):
         name = _name
         tags = _tags or []
+        changed = False
 
         def __init__(self, execute_module=None, task_vars=None, tmp=None):
             pass
@@ -22,6 +23,7 @@ def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, ru
             return is_active
 
         def run(self):
+            self.changed = changed
             if run_exception is not None:
                 raise run_exception
             return run_return
@@ -135,14 +137,15 @@ def test_action_plugin_run_check_ok(plugin, task_vars, monkeypatch):
 
 
 def test_action_plugin_run_check_changed(plugin, task_vars, monkeypatch):
-    check_return_value = {'ok': 'test', 'changed': True}
-    check_class = fake_check(run_return=check_return_value)
+    check_return_value = {'ok': 'test'}
+    check_class = fake_check(run_return=check_return_value, changed=True)
     monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {'fake_check': check_class()})
     monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
 
     result = plugin.run(tmp=None, task_vars=task_vars)
 
     assert result['checks']['fake_check'] == check_return_value
+    assert changed(result['checks']['fake_check'])
     assert not failed(result)
     assert changed(result)
     assert not skipped(result)
@@ -165,7 +168,7 @@ def test_action_plugin_run_check_fail(plugin, task_vars, monkeypatch):
 def test_action_plugin_run_check_exception(plugin, task_vars, monkeypatch):
     exception_msg = 'fake check has an exception'
     run_exception = OpenShiftCheckException(exception_msg)
-    check_class = fake_check(run_exception=run_exception)
+    check_class = fake_check(run_exception=run_exception, changed=True)
     monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {'fake_check': check_class()})
     monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
 
@@ -173,7 +176,8 @@ def test_action_plugin_run_check_exception(plugin, task_vars, monkeypatch):
 
     assert failed(result['checks']['fake_check'], msg_has=exception_msg)
     assert failed(result, msg_has=['failed'])
-    assert not changed(result)
+    assert changed(result['checks']['fake_check'])
+    assert changed(result)
     assert not skipped(result)
 
 
diff --git a/roles/openshift_health_checker/test/curator_test.py b/roles/openshift_health_checker/test/curator_test.py
index ae108c96e..62c680b74 100644
--- a/roles/openshift_health_checker/test/curator_test.py
+++ b/roles/openshift_health_checker/test/curator_test.py
@@ -1,22 +1,6 @@
 import pytest
 
-from openshift_checks.logging.curator import Curator
-
-
-def canned_curator(exec_oc=None):
-    """Create a Curator check object with canned exec_oc method"""
-    check = Curator("dummy")  # fails if a module is actually invoked
-    if exec_oc:
-        check._exec_oc = exec_oc
-    return check
-
-
-def assert_error(error, expect_error):
-    if expect_error:
-        assert error
-        assert expect_error in error
-    else:
-        assert not error
+from openshift_checks.logging.curator import Curator, OpenShiftCheckException
 
 
 plain_curator_pod = {
@@ -44,25 +28,30 @@ not_running_curator_pod = {
 }
 
 
+def test_get_curator_pods():
+    check = Curator()
+    check.get_pods_for_component = lambda *_: [plain_curator_pod]
+    result = check.run()
+    assert "failed" not in result or not result["failed"]
+
+
 @pytest.mark.parametrize('pods, expect_error', [
     (
         [],
-        "no Curator pods",
-    ),
-    (
-        [plain_curator_pod],
-        None,
+        'MissingComponentPods',
     ),
     (
         [not_running_curator_pod],
-        "not currently in a running state",
+        'CuratorNotRunning',
     ),
     (
         [plain_curator_pod, plain_curator_pod],
-        "more than one Curator pod",
+        'TooManyCurators',
     ),
 ])
-def test_get_curator_pods(pods, expect_error):
-    check = canned_curator()
-    error = check.check_curator(pods)
-    assert_error(error, expect_error)
+def test_get_curator_pods_fail(pods, expect_error):
+    check = Curator()
+    check.get_pods_for_component = lambda *_: pods
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check.run()
+    assert excinfo.value.name == expect_error
diff --git a/roles/openshift_health_checker/test/elasticsearch_test.py b/roles/openshift_health_checker/test/elasticsearch_test.py
index 67408609a..09bacd9ac 100644
--- a/roles/openshift_health_checker/test/elasticsearch_test.py
+++ b/roles/openshift_health_checker/test/elasticsearch_test.py
@@ -1,17 +1,26 @@
 import pytest
 import json
 
-from openshift_checks.logging.elasticsearch import Elasticsearch
+from openshift_checks.logging.elasticsearch import Elasticsearch, OpenShiftCheckExceptionList
+
 
 task_vars_config_base = dict(openshift=dict(common=dict(config_base='/etc/origin')))
 
 
-def assert_error(error, expect_error):
-    if expect_error:
-        assert error
-        assert expect_error in error
-    else:
-        assert not error
+def canned_elasticsearch(task_vars=None, exec_oc=None):
+    """Create an Elasticsearch check object with stubbed exec_oc method"""
+    check = Elasticsearch(None, task_vars or {})
+    if exec_oc:
+        check.exec_oc = exec_oc
+    return check
+
+
+def assert_error_in_list(expect_err, errorlist):
+    assert any(err.name == expect_err for err in errorlist), "{} in {}".format(str(expect_err), str(errorlist))
+
+
+def pods_by_name(pods):
+    return {pod['metadata']['name']: pod for pod in pods}
 
 
 plain_es_pod = {
@@ -19,6 +28,7 @@ plain_es_pod = {
         "labels": {"component": "es", "deploymentconfig": "logging-es"},
         "name": "logging-es",
     },
+    "spec": {},
     "status": {
         "conditions": [{"status": "True", "type": "Ready"}],
         "containerStatuses": [{"ready": True}],
@@ -32,6 +42,7 @@ split_es_pod = {
         "labels": {"component": "es", "deploymentconfig": "logging-es-2"},
         "name": "logging-es-2",
     },
+    "spec": {},
     "status": {
         "conditions": [{"status": "True", "type": "Ready"}],
         "containerStatuses": [{"ready": True}],
@@ -40,12 +51,28 @@ split_es_pod = {
     "_test_master_name_str": "name logging-es-2",
 }
 
+unready_es_pod = {
+    "metadata": {
+        "labels": {"component": "es", "deploymentconfig": "logging-es-3"},
+        "name": "logging-es-3",
+    },
+    "spec": {},
+    "status": {
+        "conditions": [{"status": "False", "type": "Ready"}],
+        "containerStatuses": [{"ready": False}],
+        "podIP": "10.10.10.10",
+    },
+    "_test_master_name_str": "BAD_NAME_RESPONSE",
+}
+
 
 def test_check_elasticsearch():
-    assert 'No logging Elasticsearch pods' in Elasticsearch().check_elasticsearch([])
+    with pytest.raises(OpenShiftCheckExceptionList) as excinfo:
+        canned_elasticsearch().check_elasticsearch([])
+    assert_error_in_list('NoRunningPods', excinfo.value)
 
     # canned oc responses to match so all the checks pass
-    def _exec_oc(ns, cmd, args):
+    def exec_oc(cmd, args):
         if '_cat/master' in cmd:
             return 'name logging-es'
         elif '/_nodes' in cmd:
@@ -57,35 +84,41 @@ def test_check_elasticsearch():
         else:
             raise Exception(cmd)
 
-    check = Elasticsearch(None, {})
-    check.exec_oc = _exec_oc
-    assert not check.check_elasticsearch([plain_es_pod])
+    check = canned_elasticsearch({}, exec_oc)
+    check.get_pods_for_component = lambda *_: [plain_es_pod]
+    assert {} == check.run()
 
 
-def pods_by_name(pods):
-    return {pod['metadata']['name']: pod for pod in pods}
+def test_check_running_es_pods():
+    pods, errors = Elasticsearch().running_elasticsearch_pods([plain_es_pod, unready_es_pod])
+    assert plain_es_pod in pods
+    assert_error_in_list('PodNotRunning', errors)
+
+
+def test_check_elasticsearch_masters():
+    pods = [plain_es_pod]
+    check = canned_elasticsearch(task_vars_config_base, lambda *_: plain_es_pod['_test_master_name_str'])
+    assert not check.check_elasticsearch_masters(pods_by_name(pods))
 
 
 @pytest.mark.parametrize('pods, expect_error', [
     (
         [],
-        'No logging Elasticsearch masters',
+        'NoMasterFound',
     ),
     (
-        [plain_es_pod],
-        None,
+        [unready_es_pod],
+        'NoMasterName',
     ),
     (
         [plain_es_pod, split_es_pod],
-        'Found multiple Elasticsearch masters',
+        'SplitBrainMasters',
     ),
 ])
-def test_check_elasticsearch_masters(pods, expect_error):
+def test_check_elasticsearch_masters_error(pods, expect_error):
     test_pods = list(pods)
-    check = Elasticsearch(None, task_vars_config_base)
-    check.execute_module = lambda cmd, args: {'result': test_pods.pop(0)['_test_master_name_str']}
-    errors = check._check_elasticsearch_masters(pods_by_name(pods))
-    assert_error(''.join(errors), expect_error)
+    check = canned_elasticsearch(task_vars_config_base, lambda *_: test_pods.pop(0)['_test_master_name_str'])
+    assert_error_in_list(expect_error, check.check_elasticsearch_masters(pods_by_name(pods)))
 
 
 es_node_list = {
@@ -95,83 +128,76 @@ es_node_list = {
         }}}
 
 
+def test_check_elasticsearch_node_list():
+    check = canned_elasticsearch(task_vars_config_base, lambda *_: json.dumps(es_node_list))
+    assert not check.check_elasticsearch_node_list(pods_by_name([plain_es_pod]))
+
+
 @pytest.mark.parametrize('pods, node_list, expect_error', [
     (
         [],
         {},
-        'No logging Elasticsearch masters',
-    ),
-    (
-        [plain_es_pod],
-        es_node_list,
-        None,
+        'MissingComponentPods',
     ),
     (
         [plain_es_pod],
         {},  # empty list of nodes triggers KeyError
-        "Failed to query",
+        'MissingNodeList',
     ),
     (
         [split_es_pod],
         es_node_list,
-        'does not correspond to any known ES pod',
+        'EsPodNodeMismatch',
     ),
 ])
-def test_check_elasticsearch_node_list(pods, node_list, expect_error):
-    check = Elasticsearch(None, task_vars_config_base)
-    check.execute_module = lambda cmd, args: {'result': json.dumps(node_list)}
+def test_check_elasticsearch_node_list_errors(pods, node_list, expect_error):
+    check = canned_elasticsearch(task_vars_config_base, lambda cmd, args: json.dumps(node_list))
+    assert_error_in_list(expect_error, check.check_elasticsearch_node_list(pods_by_name(pods)))
 
-    errors = check._check_elasticsearch_node_list(pods_by_name(pods))
-    assert_error(''.join(errors), expect_error)
+
+def test_check_elasticsearch_cluster_health():
+    test_health_data = [{"status": "green"}]
+    check = canned_elasticsearch(exec_oc=lambda *_: json.dumps(test_health_data.pop(0)))
+    assert not check.check_es_cluster_health(pods_by_name([plain_es_pod]))
 
 
 @pytest.mark.parametrize('pods, health_data, expect_error', [
     (
         [plain_es_pod],
-        [{"status": "green"}],
-        None,
-    ),
-    (
-        [plain_es_pod],
         [{"no-status": "should bomb"}],
-        'Could not retrieve cluster health status',
+        'BadEsResponse',
     ),
     (
         [plain_es_pod, split_es_pod],
         [{"status": "green"}, {"status": "red"}],
-        'Elasticsearch cluster health status is RED',
+        'EsClusterHealthRed',
     ),
 ])
-def test_check_elasticsearch_cluster_health(pods, health_data, expect_error):
+def test_check_elasticsearch_cluster_health_errors(pods, health_data, expect_error):
     test_health_data = list(health_data)
-    check = Elasticsearch(None, task_vars_config_base)
-    check.execute_module = lambda cmd, args: {'result': json.dumps(test_health_data.pop(0))}
+    check = canned_elasticsearch(exec_oc=lambda *_: json.dumps(test_health_data.pop(0)))
+    assert_error_in_list(expect_error, check.check_es_cluster_health(pods_by_name(pods)))
 
-    errors = check._check_es_cluster_health(pods_by_name(pods))
-    assert_error(''.join(errors), expect_error)
+
+def test_check_elasticsearch_diskspace():
+    check = canned_elasticsearch(exec_oc=lambda *_: 'IUse% Use%\n 3%  4%\n')
+    assert not check.check_elasticsearch_diskspace(pods_by_name([plain_es_pod]))
 
 
 @pytest.mark.parametrize('disk_data, expect_error', [
     (
         'df: /elasticsearch/persistent: No such file or directory\n',
-        'Could not retrieve storage usage',
-    ),
-    (
-        'IUse% Use%\n 3%  4%\n',
-        None,
+        'BadDfResponse',
     ),
     (
         'IUse% Use%\n 95%  40%\n',
-        'Inode percent usage on the storage volume',
+        'InodeUsageTooHigh',
     ),
     (
         'IUse% Use%\n 3%  94%\n',
-        'Disk percent usage on the storage volume',
+        'DiskUsageTooHigh',
     ),
 ])
-def test_check_elasticsearch_diskspace(disk_data, expect_error):
-    check = Elasticsearch(None, task_vars_config_base)
-    check.execute_module = lambda cmd, args: {'result': disk_data}
-
-    errors = check._check_elasticsearch_diskspace(pods_by_name([plain_es_pod]))
-    assert_error(''.join(errors), expect_error)
+def test_check_elasticsearch_diskspace_errors(disk_data, expect_error):
+    check = canned_elasticsearch(exec_oc=lambda *_: disk_data)
+    assert_error_in_list(expect_error, check.check_elasticsearch_diskspace(pods_by_name([plain_es_pod])))
diff --git a/roles/openshift_health_checker/test/fluentd_config_test.py b/roles/openshift_health_checker/test/fluentd_config_test.py
index 8a2d8b72b..10db253bc 100644
--- a/roles/openshift_health_checker/test/fluentd_config_test.py
+++ b/roles/openshift_health_checker/test/fluentd_config_test.py
@@ -198,12 +198,9 @@ def test_check_logging_config_master(name, pods, logging_driver, extra_words):
         ),
     )
 
-    def get_pods(namespace, logging_component):
-        return pods, None
-
     check = FluentdConfig(execute_module, task_vars)
     check.execute_module = execute_module
-    check.get_pods_for_component = get_pods
+    check.get_pods_for_component = lambda _: pods
     error = check.check_logging_config()
 
     assert error is None
@@ -283,12 +280,9 @@ def test_check_logging_config_master_failed(name, pods, logging_driver, words):
         ),
     )
 
-    def get_pods(namespace, logging_component):
-        return pods, None
-
     check = FluentdConfig(execute_module, task_vars)
     check.execute_module = execute_module
-    check.get_pods_for_component = get_pods
+    check.get_pods_for_component = lambda _: pods
     error = check.check_logging_config()
 
     assert error is not None
@@ -343,11 +337,8 @@ def test_check_logging_config_master_fails_on_unscheduled_deployment(name, pods,
         ),
     )
 
-    def get_pods(namespace, logging_component):
-        return pods, None
-
     check = FluentdConfig(execute_module, task_vars)
-    check.get_pods_for_component = get_pods
+    check.get_pods_for_component = lambda _: pods
 
     with pytest.raises(OpenShiftCheckException) as error:
         check.check_logging_config()
diff --git a/roles/openshift_health_checker/test/fluentd_test.py b/roles/openshift_health_checker/test/fluentd_test.py
index a84d89cef..e7bf9818b 100644
--- a/roles/openshift_health_checker/test/fluentd_test.py
+++ b/roles/openshift_health_checker/test/fluentd_test.py
@@ -1,15 +1,11 @@
 import pytest
 import json
 
-from openshift_checks.logging.fluentd import Fluentd
+from openshift_checks.logging.fluentd import Fluentd, OpenShiftCheckExceptionList, OpenShiftCheckException
 
 
-def assert_error(error, expect_error):
-    if expect_error:
-        assert error
-        assert expect_error in error
-    else:
-        assert not error
+def assert_error_in_list(expect_err, errorlist):
+    assert any(err.name == expect_err for err in errorlist), "{} in {}".format(str(expect_err), str(errorlist))
 
 
 fluentd_pod_node1 = {
@@ -57,45 +53,60 @@ fluentd_node3_unlabeled = {
 }
 
 
+def test_get_fluentd_pods():
+    check = Fluentd()
+    check.exec_oc = lambda *_: json.dumps(dict(items=[fluentd_node1]))
+    check.get_pods_for_component = lambda *_: [fluentd_pod_node1]
+    assert not check.run()
+
+
 @pytest.mark.parametrize('pods, nodes, expect_error', [
     (
         [],
         [],
-        'No nodes appear to be defined',
+        'NoNodesDefined',
     ),
     (
         [],
         [fluentd_node3_unlabeled],
-        'There are no nodes with the fluentd label',
+        'NoNodesLabeled',
     ),
     (
         [],
         [fluentd_node1, fluentd_node3_unlabeled],
-        'Fluentd will not aggregate logs from these nodes.',
+        'NodesUnlabeled',
     ),
     (
         [],
         [fluentd_node2],
-        "nodes are supposed to have a Fluentd pod but do not",
+        'MissingFluentdPod',
     ),
     (
         [fluentd_pod_node1, fluentd_pod_node1],
         [fluentd_node1],
-        'more Fluentd pods running than nodes labeled',
+        'TooManyFluentdPods',
     ),
     (
         [fluentd_pod_node2_down],
         [fluentd_node2],
-        "Fluentd pods are supposed to be running",
-    ),
-    (
-        [fluentd_pod_node1],
-        [fluentd_node1],
-        None,
+        'FluentdNotRunning',
     ),
 ])
-def test_get_fluentd_pods(pods, nodes, expect_error):
+def test_get_fluentd_pods_errors(pods, nodes, expect_error):
+    check = Fluentd()
+    check.exec_oc = lambda *_: json.dumps(dict(items=nodes))
+
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check.check_fluentd(pods)
+    if isinstance(excinfo.value, OpenShiftCheckExceptionList):
+        assert_error_in_list(expect_error, excinfo.value)
+    else:
+        assert expect_error == excinfo.value.name
+
+
+def test_bad_oc_node_list():
     check = Fluentd()
-    check.exec_oc = lambda ns, cmd, args: json.dumps(dict(items=nodes))
-    error = check.check_fluentd(pods)
-    assert_error(error, expect_error)
+    check.exec_oc = lambda *_: "this isn't even json"
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check.get_nodes_by_name()
+    assert 'BadOcNodeList' == excinfo.value.name
diff --git a/roles/openshift_health_checker/test/kibana_test.py b/roles/openshift_health_checker/test/kibana_test.py
index 0bf492511..04a5e89c4 100644
--- a/roles/openshift_health_checker/test/kibana_test.py
+++ b/roles/openshift_health_checker/test/kibana_test.py
@@ -8,15 +8,7 @@ except ImportError:
     from urllib.error import HTTPError, URLError
     import urllib.request as urllib2
 
-from openshift_checks.logging.kibana import Kibana
-
-
-def assert_error(error, expect_error):
-    if expect_error:
-        assert error
-        assert expect_error in error
-    else:
-        assert not error
+from openshift_checks.logging.kibana import Kibana, OpenShiftCheckException
 
 
 plain_kibana_pod = {
@@ -41,39 +33,45 @@ not_running_kibana_pod = {
 }
 
 
+def test_check_kibana():
+    # should run without exception:
+    Kibana().check_kibana([plain_kibana_pod])
+
+
 @pytest.mark.parametrize('pods, expect_error', [
     (
         [],
-        "There are no Kibana pods deployed",
-    ),
-    (
-        [plain_kibana_pod],
-        None,
+        "MissingComponentPods",
     ),
     (
         [not_running_kibana_pod],
-        "No Kibana pod is in a running state",
+        "NoRunningPods",
     ),
     (
         [plain_kibana_pod, not_running_kibana_pod],
-        "The following Kibana pods are not currently in a running state",
+        "PodNotRunning",
     ),
 ])
-def test_check_kibana(pods, expect_error):
-    check = Kibana()
-    error = check.check_kibana(pods)
-    assert_error(error, expect_error)
+def test_check_kibana_error(pods, expect_error):
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        Kibana().check_kibana(pods)
+    assert expect_error == excinfo.value.name
 
 
-@pytest.mark.parametrize('route, expect_url, expect_error', [
+@pytest.mark.parametrize('comment, route, expect_error', [
     (
+        "No route returned",
         None,
-        None,
-        'no_route_exists',
+        "no_route_exists",
     ),
 
-    # test route with no ingress
     (
+        "broken route response",
+        {"status": {}},
+        "get_route_failed",
+    ),
+    (
+        "route with no ingress",
         {
             "metadata": {
                 "labels": {"component": "kibana", "deploymentconfig": "logging-kibana"},
@@ -86,12 +84,11 @@ def test_check_kibana(pods, expect_error):
                 "host": "hostname",
             }
         },
-        None,
-        'route_not_accepted',
+        "route_not_accepted",
     ),
 
-    # test route with no host
     (
+        "route with no host",
         {
             "metadata": {
                 "labels": {"component": "kibana", "deploymentconfig": "logging-kibana"},
@@ -104,12 +101,21 @@ def test_check_kibana(pods, expect_error):
             },
             "spec": {},
         },
-        None,
-        'route_missing_host',
+        "route_missing_host",
     ),
+])
+def test_get_kibana_url_error(comment, route, expect_error):
+    check = Kibana()
+    check.exec_oc = lambda *_: json.dumps(route) if route else ""
+
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check._get_kibana_url()
+    assert excinfo.value.name == expect_error
 
-    # test route that looks fine
+
+@pytest.mark.parametrize('comment, route, expect_url', [
     (
+        "test route that looks fine",
         {
             "metadata": {
                 "labels": {"component": "kibana", "deploymentconfig": "logging-kibana"},
@@ -125,62 +131,57 @@ def test_check_kibana(pods, expect_error):
             },
         },
         "https://hostname/",
-        None,
     ),
 ])
-def test_get_kibana_url(route, expect_url, expect_error):
+def test_get_kibana_url(comment, route, expect_url):
     check = Kibana()
-    check.exec_oc = lambda ns, cmd, args: json.dumps(route) if route else ""
-
-    url, error = check._get_kibana_url()
-    if expect_url:
-        assert url == expect_url
-    else:
-        assert not url
-    if expect_error:
-        assert error == expect_error
-    else:
-        assert not error
+    check.exec_oc = lambda *_: json.dumps(route)
+    assert expect_url == check._get_kibana_url()
 
 
 @pytest.mark.parametrize('exec_result, expect', [
     (
         'urlopen error [Errno 111] Connection refused',
-        'at least one router routing to it?',
+        'FailedToConnectInternal',
     ),
     (
         'urlopen error [Errno -2] Name or service not known',
-        'DNS configured for the Kibana hostname?',
+        'FailedToResolveInternal',
     ),
     (
         'Status code was not [302]: HTTP Error 500: Server error',
-        'did not return the correct status code',
+        'WrongReturnCodeInternal',
     ),
     (
         'bork bork bork',
-        'bork bork bork',  # should pass through
+        'MiscRouteErrorInternal',
     ),
 ])
 def test_verify_url_internal_failure(exec_result, expect):
     check = Kibana(execute_module=lambda *_: dict(failed=True, msg=exec_result))
-    check._get_kibana_url = lambda: ('url', None)
+    check._get_kibana_url = lambda: 'url'
 
-    error = check._check_kibana_route()
-    assert_error(error, expect)
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check.check_kibana_route()
+    assert expect == excinfo.value.name
 
 
 @pytest.mark.parametrize('lib_result, expect', [
     (
-        HTTPError('url', 500, "it broke", hdrs=None, fp=None),
-        'it broke',
+        HTTPError('url', 500, 'it broke', hdrs=None, fp=None),
+        'MiscRouteError',
+    ),
+    (
+        URLError('urlopen error [Errno 111] Connection refused'),
+        'FailedToConnect',
     ),
     (
-        URLError('it broke'),
-        'it broke',
+        URLError('urlopen error [Errno -2] Name or service not known'),
+        'FailedToResolve',
     ),
     (
         302,
-        'returned the wrong error code',
+        'WrongReturnCode',
     ),
     (
         200,
@@ -204,8 +205,40 @@ def test_verify_url_external_failure(lib_result, expect, monkeypatch):
     monkeypatch.setattr(urllib2, 'urlopen', urlopen)
 
     check = Kibana()
-    check._get_kibana_url = lambda: ('url', None)
+    check._get_kibana_url = lambda: 'url'
     check._verify_url_internal = lambda url: None
 
-    error = check._check_kibana_route()
-    assert_error(error, expect)
+    if not expect:
+        check.check_kibana_route()
+        return
+
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check.check_kibana_route()
+    assert expect == excinfo.value.name
+
+
+def test_verify_url_external_skip():
+    check = Kibana(lambda *_: {}, dict(openshift_check_efk_kibana_external="false"))
+    check._get_kibana_url = lambda: 'url'
+    check.check_kibana_route()
+
+
+# this is kind of silly but it adds coverage for the run() method...
+def test_run():
+    pods = ["foo"]
+    ran = dict(check_kibana=False, check_route=False)
+
+    def check_kibana(pod_list):
+        ran["check_kibana"] = True
+        assert pod_list == pods
+
+    def check_kibana_route():
+        ran["check_route"] = True
+
+    check = Kibana()
+    check.get_pods_for_component = lambda *_: pods
+    check.check_kibana = check_kibana
+    check.check_kibana_route = check_kibana_route
+
+    check.run()
+    assert ran["check_kibana"] and ran["check_route"]
diff --git a/roles/openshift_health_checker/test/logging_check_test.py b/roles/openshift_health_checker/test/logging_check_test.py
index 6f1697ee6..1a1c190f6 100644
--- a/roles/openshift_health_checker/test/logging_check_test.py
+++ b/roles/openshift_health_checker/test/logging_check_test.py
@@ -1,18 +1,14 @@
 import pytest
 import json
 
-from openshift_checks.logging.logging import LoggingCheck, OpenShiftCheckException
+from openshift_checks.logging.logging import LoggingCheck, MissingComponentPods, CouldNotUseOc
 
 task_vars_config_base = dict(openshift=dict(common=dict(config_base='/etc/origin')))
 
 
-logging_namespace = "logging"
-
-
-def canned_loggingcheck(exec_oc=None):
+def canned_loggingcheck(exec_oc=None, execute_module=None):
     """Create a LoggingCheck object with canned exec_oc method"""
-    check = LoggingCheck()  # fails if a module is actually invoked
-    check.logging_namespace = 'logging'
+    check = LoggingCheck(execute_module)
     if exec_oc:
         check.exec_oc = exec_oc
     return check
@@ -97,8 +93,8 @@ def test_oc_failure(problem, expect):
 
     check = LoggingCheck(execute_module, task_vars_config_base)
 
-    with pytest.raises(OpenShiftCheckException) as excinfo:
-        check.exec_oc(logging_namespace, 'get foo', [])
+    with pytest.raises(CouldNotUseOc) as excinfo:
+        check.exec_oc('get foo', [])
     assert expect in str(excinfo)
 
 
@@ -124,25 +120,32 @@ def test_is_active(groups, logging_deployed, is_active):
     assert LoggingCheck(None, task_vars).is_active() == is_active
 
 
-@pytest.mark.parametrize('pod_output, expect_pods, expect_error', [
+@pytest.mark.parametrize('pod_output, expect_pods', [
+    (
+        json.dumps({'items': [plain_es_pod]}),
+        [plain_es_pod],
+    ),
+])
+def test_get_pods_for_component(pod_output, expect_pods):
+    check = canned_loggingcheck(lambda *_: pod_output)
+    pods = check.get_pods_for_component("es")
+    assert pods == expect_pods
+
+
+@pytest.mark.parametrize('exec_oc_output, expect_error', [
     (
         'No resources found.',
-        None,
-        'No pods were found for the "es"',
+        MissingComponentPods,
     ),
     (
-        json.dumps({'items': [plain_kibana_pod, plain_es_pod, plain_curator_pod, fluentd_pod_node1]}),
-        [plain_es_pod],
-        None,
+        '{"items": null}',
+        MissingComponentPods,
     ),
 ])
-def test_get_pods_for_component(pod_output, expect_pods, expect_error):
-    check = canned_loggingcheck(lambda namespace, cmd, args: pod_output)
-    pods, error = check.get_pods_for_component(
-        logging_namespace,
-        "es",
-    )
-    assert_error(error, expect_error)
+def test_get_pods_for_component_fail(exec_oc_output, expect_error):
+    check = canned_loggingcheck(lambda *_: exec_oc_output)
+    with pytest.raises(expect_error):
+        check.get_pods_for_component("es")
 
 
 @pytest.mark.parametrize('name, pods, expected_pods', [
@@ -159,7 +162,7 @@ def test_get_pods_for_component(pod_output, expect_pods, expect_error):
 
 ], ids=lambda argvals: argvals[0])
 def test_get_not_running_pods_no_container_status(name, pods, expected_pods):
-    check = canned_loggingcheck(lambda exec_module, namespace, cmd, args, task_vars: '')
+    check = canned_loggingcheck(lambda *_: '')
     result = check.not_running_pods(pods)
 
     assert result == expected_pods
diff --git a/roles/openshift_health_checker/test/logging_index_time_test.py b/roles/openshift_health_checker/test/logging_index_time_test.py
index 178d7cd84..22566b295 100644
--- a/roles/openshift_health_checker/test/logging_index_time_test.py
+++ b/roles/openshift_health_checker/test/logging_index_time_test.py
@@ -69,7 +69,29 @@ def test_check_running_pods(pods, expect_pods):
     assert pods == expect_pods
 
 
-@pytest.mark.parametrize('name, json_response, uuid, timeout, extra_words', [
+def test_bad_config_param():
+    with pytest.raises(OpenShiftCheckException) as error:
+        LoggingIndexTime(task_vars=dict(openshift_check_logging_index_timeout_seconds="foo")).run()
+    assert 'InvalidTimeout' == error.value.name
+
+
+def test_no_running_pods():
+    check = LoggingIndexTime()
+    check.get_pods_for_component = lambda *_: [not_running_kibana_pod]
+    with pytest.raises(OpenShiftCheckException) as error:
+        check.run()
+    assert 'kibanaNoRunningPods' == error.value.name
+
+
+def test_with_running_pods():
+    check = LoggingIndexTime()
+    check.get_pods_for_component = lambda *_: [plain_running_kibana_pod, plain_running_elasticsearch_pod]
+    check.curl_kibana_with_uuid = lambda *_: SAMPLE_UUID
+    check.wait_until_cmd_or_err = lambda *_: None
+    assert not check.run().get("failed")
+
+
+@pytest.mark.parametrize('name, json_response, uuid, timeout', [
     (
         'valid count in response',
         {
@@ -77,94 +99,72 @@ def test_check_running_pods(pods, expect_pods):
         },
         SAMPLE_UUID,
         0.001,
-        [],
     ),
 ], ids=lambda argval: argval[0])
-def test_wait_until_cmd_or_err_succeeds(name, json_response, uuid, timeout, extra_words):
+def test_wait_until_cmd_or_err_succeeds(name, json_response, uuid, timeout):
     check = canned_loggingindextime(lambda *_: json.dumps(json_response))
     check.wait_until_cmd_or_err(plain_running_elasticsearch_pod, uuid, timeout)
 
 
-@pytest.mark.parametrize('name, json_response, uuid, timeout, extra_words', [
+@pytest.mark.parametrize('name, json_response, timeout, expect_error', [
     (
         'invalid json response',
         {
             "invalid_field": 1,
         },
-        SAMPLE_UUID,
         0.001,
-        ["invalid response", "Elasticsearch"],
+        'esInvalidResponse',
     ),
     (
         'empty response',
         {},
-        SAMPLE_UUID,
         0.001,
-        ["invalid response", "Elasticsearch"],
+        'esInvalidResponse',
     ),
     (
         'valid response but invalid match count',
         {
             "count": 0,
         },
-        SAMPLE_UUID,
         0.005,
-        ["expecting match", SAMPLE_UUID, "0.005s"],
+        'NoMatchFound',
     )
 ], ids=lambda argval: argval[0])
-def test_wait_until_cmd_or_err(name, json_response, uuid, timeout, extra_words):
+def test_wait_until_cmd_or_err(name, json_response, timeout, expect_error):
     check = canned_loggingindextime(lambda *_: json.dumps(json_response))
     with pytest.raises(OpenShiftCheckException) as error:
-        check.wait_until_cmd_or_err(plain_running_elasticsearch_pod, uuid, timeout)
+        check.wait_until_cmd_or_err(plain_running_elasticsearch_pod, SAMPLE_UUID, timeout)
 
-    for word in extra_words:
-        assert word in str(error)
+    assert expect_error == error.value.name
 
 
-@pytest.mark.parametrize('name, json_response, uuid, extra_words', [
-    (
-        'correct response code, found unique id is returned',
-        {
-            "statusCode": 404,
-        },
-        "sample unique id",
-        ["sample unique id"],
-    ),
-], ids=lambda argval: argval[0])
-def test_curl_kibana_with_uuid(name, json_response, uuid, extra_words):
-    check = canned_loggingindextime(lambda *_: json.dumps(json_response))
-    check.generate_uuid = lambda: uuid
-
-    result = check.curl_kibana_with_uuid(plain_running_kibana_pod)
-
-    for word in extra_words:
-        assert word in result
+def test_curl_kibana_with_uuid():
+    check = canned_loggingindextime(lambda *_: json.dumps({"statusCode": 404}))
+    check.generate_uuid = lambda: SAMPLE_UUID
+    assert SAMPLE_UUID == check.curl_kibana_with_uuid(plain_running_kibana_pod)
 
 
-@pytest.mark.parametrize('name, json_response, uuid, extra_words', [
+@pytest.mark.parametrize('name, json_response, expect_error', [
     (
         'invalid json response',
         {
             "invalid_field": "invalid",
         },
-        SAMPLE_UUID,
-        ["invalid response returned", 'Missing "statusCode" key'],
+        'kibanaInvalidResponse',
     ),
     (
         'wrong error code in response',
         {
             "statusCode": 500,
         },
-        SAMPLE_UUID,
-        ["Expecting error code", "500"],
+        'kibanaInvalidReturnCode',
     ),
 ], ids=lambda argval: argval[0])
-def test_failed_curl_kibana_with_uuid(name, json_response, uuid, extra_words):
+def test_failed_curl_kibana_with_uuid(name, json_response, expect_error):
     check = canned_loggingindextime(lambda *_: json.dumps(json_response))
-    check.generate_uuid = lambda: uuid
+    check.generate_uuid = lambda: SAMPLE_UUID
 
     with pytest.raises(OpenShiftCheckException) as error:
         check.curl_kibana_with_uuid(plain_running_kibana_pod)
 
-    for word in extra_words:
-        assert word in str(error)
+    assert expect_error == error.value.name