From d20c3caa6bd760afaed1c4a3644a73ea64b7c05f Mon Sep 17 00:00:00 2001 From: David Martin Date: Thu, 21 Nov 2019 21:58:58 +0100 Subject: [PATCH 1/2] First draft for global syntax validation - report all validation errors at once Signed-off-by: David Martin --- chaoslib/activity.py | 58 ++++++++++++++------------ chaoslib/control/__init__.py | 29 +++++++------ chaoslib/control/python.py | 11 +++-- chaoslib/experiment.py | 54 ++++++++++++++---------- chaoslib/extension.py | 14 ++++--- chaoslib/hypothesis.py | 19 ++++++--- chaoslib/provider/http.py | 17 +++++--- chaoslib/provider/process.py | 30 +++++++++----- chaoslib/provider/python.py | 47 +++++++++++++-------- tests/test_action.py | 6 +-- tests/test_control.py | 5 ++- tests/test_experiment.py | 2 +- tests/test_extension.py | 8 ++-- tests/test_probe.py | 80 +++++++++++++++++++----------------- 14 files changed, 224 insertions(+), 156 deletions(-) diff --git a/chaoslib/activity.py b/chaoslib/activity.py index 0e242ec..689ce3a 100644 --- a/chaoslib/activity.py +++ b/chaoslib/activity.py @@ -36,67 +36,73 @@ def ensure_activity_is_valid(activity: Activity): In all failing cases, raises :exc:`InvalidActivity`. """ + errors = [] if not activity: - raise InvalidActivity("empty activity is no activity") + errors.append(InvalidActivity("empty activity is no activity")) + return errors # when the activity is just a ref, there is little to validate ref = activity.get("ref") if ref is not None: if not isinstance(ref, str) or ref == '': - raise InvalidActivity( - "reference to activity must be non-empty strings") - return + errors.append(InvalidActivity( + "reference to activity must be non-empty strings")) + return errors activity_type = activity.get("type") if not activity_type: - raise InvalidActivity("an activity must have a type") + errors.append(InvalidActivity("an activity must have a type")) if activity_type not in ("probe", "action"): - raise InvalidActivity( - "'{t}' is not a supported activity type".format(t=activity_type)) + errors.append(InvalidActivity( + "'{t}' is not a supported activity type".format(t=activity_type))) if not activity.get("name"): - raise InvalidActivity("an activity must have a name") + errors.append(InvalidActivity("an activity must have a name")) provider = activity.get("provider") if not provider: - raise InvalidActivity("an activity requires a provider") + errors.append(InvalidActivity("an activity requires a provider")) + provider_type = None + else: + provider_type = provider.get("type") + if not provider_type: + errors.append(InvalidActivity("a provider must have a type")) - provider_type = provider.get("type") - if not provider_type: - raise InvalidActivity("a provider must have a type") - - if provider_type not in ("python", "process", "http"): - raise InvalidActivity( - "unknown provider type '{type}'".format(type=provider_type)) - - if not activity.get("name"): - raise InvalidActivity("activity must have a name (cannot be empty)") + if provider_type not in ("python", "process", "http"): + errors.append(InvalidActivity( + "unknown provider type '{type}'".format(type=provider_type))) timeout = activity.get("timeout") if timeout is not None: if not isinstance(timeout, numbers.Number): - raise InvalidActivity("activity timeout must be a number") + errors.append( + InvalidActivity("activity timeout must be a number")) pauses = activity.get("pauses") if pauses is not None: before = pauses.get("before") if before is not None and not isinstance(before, numbers.Number): - raise InvalidActivity("activity before pause must be a number") + errors.append( + InvalidActivity("activity before pause must be a number")) after = pauses.get("after") if after is not None and not isinstance(after, numbers.Number): - raise InvalidActivity("activity after pause must be a number") + errors.append( + InvalidActivity("activity after pause must be a number")) if "background" in activity: if not isinstance(activity["background"], bool): - raise InvalidActivity("activity background must be a boolean") + errors.append( + InvalidActivity("activity background must be a boolean")) if provider_type == "python": - validate_python_activity(activity) + errors.extend(validate_python_activity(activity)) elif provider_type == "process": - validate_process_activity(activity) + errors.extend(validate_process_activity(activity)) elif provider_type == "http": - validate_http_activity(activity) + errors.extend(validate_http_activity(activity)) + + return errors def run_activities(experiment: Experiment, configuration: Configuration, diff --git a/chaoslib/control/__init__.py b/chaoslib/control/__init__.py index 48db5b0..10220bd 100644 --- a/chaoslib/control/__init__.py +++ b/chaoslib/control/__init__.py @@ -7,7 +7,8 @@ from chaoslib.control.python import apply_python_control, cleanup_control, \ initialize_control, validate_python_control, import_control -from chaoslib.exceptions import InterruptExecution, InvalidControl +from chaoslib.exceptions import InterruptExecution, InvalidControl, \ + ChaosException from chaoslib.settings import get_loaded_settings from chaoslib.types import Settings from chaoslib.types import Activity, Configuration, Control as ControlType, \ @@ -85,12 +86,11 @@ def cleanup_controls(experiment: Experiment): cleanup_control(control) -def validate_controls(experiment: Experiment): +def validate_controls(experiment: Experiment) -> List[ChaosException]: """ Validate that all declared controls respect the specification. - - Raises :exc:`chaoslib.exceptions.InvalidControl` when they are not valid. """ + errors = [] controls = get_controls(experiment) references = [ c["name"] for c in get_controls(experiment) @@ -99,26 +99,29 @@ def validate_controls(experiment: Experiment): for c in controls: if "ref" in c: if c["ref"] not in references: - raise InvalidControl( - "Control reference '{}' declaration cannot be found") + errors.append(InvalidControl( + "Control reference '{}' declaration cannot be found")) if "name" not in c: - raise InvalidControl("A control must have a `name` property") + errors.append( + InvalidControl("A control must have a `name` property")) - name = c["name"] + name = c.get("name", '') if "provider" not in c: - raise InvalidControl( - "Control '{}' must have a `provider` property".format(name)) + errors.append(InvalidControl( + "Control '{}' must have a `provider` property".format(name))) scope = c.get("scope") if scope and scope not in ("before", "after"): - raise InvalidControl( + errors.append(InvalidControl( "Control '{}' scope property must be 'before' or " - "'after' only".format(name)) + "'after' only".format(name))) provider_type = c.get("provider", {}).get("type") if provider_type == "python": - validate_python_control(c) + errors.extend(validate_python_control(c)) + + return errors def initialize_global_controls(experiment: Experiment, diff --git a/chaoslib/control/python.py b/chaoslib/control/python.py index 7b3071f..6570fa0 100644 --- a/chaoslib/control/python.py +++ b/chaoslib/control/python.py @@ -7,7 +7,7 @@ from logzero import logger from chaoslib import substitute -from chaoslib.exceptions import InvalidActivity +from chaoslib.exceptions import InvalidActivity, ChaosException from chaoslib.types import Activity, Configuration, Control, Experiment, \ Journal, Run, Secrets, Settings @@ -83,16 +83,19 @@ def cleanup_control(control: Control): func() -def validate_python_control(control: Control): +def validate_python_control(control: Control) -> List[ChaosException]: """ Verify that a control block matches the specification """ + errors = [] name = control["name"] provider = control["provider"] mod_name = provider.get("module") if not mod_name: - raise InvalidActivity( - "Control '{}' must have a module path".format(name)) + errors.append(InvalidActivity( + "Control '{}' must have a module path".format(name))) + # can not continue any longer - must exit this function + return errors try: importlib.import_module(mod_name) diff --git a/chaoslib/experiment.py b/chaoslib/experiment.py index 11bc925..1805eb0 100644 --- a/chaoslib/experiment.py +++ b/chaoslib/experiment.py @@ -48,54 +48,66 @@ def ensure_experiment_is_valid(experiment: Experiment): another set of of ̀close` probes to sense the state of the system post-action. - This function raises :exc:`InvalidExperiment`, :exc:`InvalidProbe` or - :exc:`InvalidAction` depending on where it fails. + This function raises an :exc:`InvalidExperiment` error + if the experiment is not valid. + If multiple validation errors are found, the errors are listed + as part of the exception message """ logger.info("Validating the experiment's syntax") + errors = [] + if not experiment: raise InvalidExperiment("an empty experiment is not an experiment") if not experiment.get("title"): - raise InvalidExperiment("experiment requires a title") + errors.append(InvalidExperiment("experiment requires a title")) if not experiment.get("description"): - raise InvalidExperiment("experiment requires a description") + errors.append(InvalidExperiment("experiment requires a description")) tags = experiment.get("tags") if tags: if list(filter(lambda t: t == '' or not isinstance(t, str), tags)): - raise InvalidExperiment( - "experiment tags must be a non-empty string") + errors.append(InvalidExperiment( + "experiment tags must be a non-empty string")) - validate_extensions(experiment) + errors.extend(validate_extensions(experiment)) config = load_configuration(experiment.get("configuration", {})) load_secrets(experiment.get("secrets", {}), config) - ensure_hypothesis_is_valid(experiment) + errors.extend(ensure_hypothesis_is_valid(experiment)) method = experiment.get("method") if not method: - raise InvalidExperiment("an experiment requires a method with " - "at least one activity") - - for activity in method: - ensure_activity_is_valid(activity) - - # let's see if a ref is indeed found in the experiment - ref = activity.get("ref") - if ref and not lookup_activity(ref): - raise InvalidActivity("referenced activity '{r}' could not be " - "found in the experiment".format(r=ref)) + errors.append(InvalidExperiment("an experiment requires a method with " + "at least one activity")) + else: + for activity in method: + errors.extend(ensure_activity_is_valid(activity)) + + # let's see if a ref is indeed found in the experiment + ref = activity.get("ref") + if ref and not lookup_activity(ref): + errors.append( + InvalidActivity("referenced activity '{r}' could not be " + "found in the experiment".format(r=ref))) rollbacks = experiment.get("rollbacks", []) for activity in rollbacks: - ensure_activity_is_valid(activity) + errors.extend(ensure_activity_is_valid(activity)) warn_about_deprecated_features(experiment) - validate_controls(experiment) + errors.extend(validate_controls(experiment)) + + if errors: + full_validation_msg = 'Experiment is not valid, ' \ + 'please fix the following errors:' + for error in errors: + full_validation_msg += '\n- {}'.format(error) + raise InvalidExperiment(full_validation_msg) logger.info("Experiment looks valid") diff --git a/chaoslib/extension.py b/chaoslib/extension.py index 2f8c798..bb28aa8 100644 --- a/chaoslib/extension.py +++ b/chaoslib/extension.py @@ -1,25 +1,29 @@ # -*- coding: utf-8 -*- -from typing import Optional +from typing import Optional, List -from chaoslib.exceptions import InvalidExperiment +from chaoslib.exceptions import InvalidExperiment, ChaosException from chaoslib.types import Experiment, Extension __all__ = ["get_extension", "has_extension", "set_extension", "merge_extension", "remove_extension", "validate_extensions"] -def validate_extensions(experiment: Experiment): +def validate_extensions(experiment: Experiment) -> List[ChaosException]: """ Validate that extensions respect the specification. """ extensions = experiment.get("extensions") if not extensions: - return + return [] + errors = [] for ext in extensions: ext_name = ext.get('name') if not ext_name or not ext_name.strip(): - raise InvalidExperiment("All extensions require a non-empty name") + errors.append( + InvalidExperiment("All extensions require a non-empty name")) + + return errors def get_extension(experiment: Experiment, name: str) -> Optional[Extension]: diff --git a/chaoslib/hypothesis.py b/chaoslib/hypothesis.py index 9badc27..9580069 100644 --- a/chaoslib/hypothesis.py +++ b/chaoslib/hypothesis.py @@ -34,21 +34,25 @@ def ensure_hypothesis_is_valid(experiment: Experiment): """ hypo = experiment.get("steady-state-hypothesis") if hypo is None: - return + return [] + errors = [] if not hypo.get("title"): - raise InvalidExperiment("hypothesis requires a title") + errors.append(InvalidExperiment("hypothesis requires a title")) probes = hypo.get("probes") if probes: for probe in probes: - ensure_activity_is_valid(probe) + errors.extend(ensure_activity_is_valid(probe)) if "tolerance" not in probe: - raise InvalidActivity( - "hypothesis probe must have a tolerance entry") + errors.append(InvalidActivity( + "hypothesis probe must have a tolerance entry")) + else: + errors.extend( + ensure_hypothesis_tolerance_is_valid(probe["tolerance"])) - ensure_hypothesis_tolerance_is_valid(probe["tolerance"]) + return errors def ensure_hypothesis_tolerance_is_valid(tolerance: Tolerance): @@ -81,6 +85,9 @@ def ensure_hypothesis_tolerance_is_valid(tolerance: Tolerance): "hypothesis probe tolerance type '{}' is unsupported".format( tolerance_type)) + # TODO + return [] + def check_regex_pattern(tolerance: Tolerance): """ diff --git a/chaoslib/provider/http.py b/chaoslib/provider/http.py index 4954156..2af37f5 100644 --- a/chaoslib/provider/http.py +++ b/chaoslib/provider/http.py @@ -1,12 +1,12 @@ # -*- coding: utf-8 -*- -from typing import Any +from typing import Any, List import urllib3 from logzero import logger import requests from chaoslib import substitute -from chaoslib.exceptions import ActivityFailed, InvalidActivity +from chaoslib.exceptions import ActivityFailed, InvalidActivity, ChaosException from chaoslib.types import Activity, Configuration, Secrets @@ -89,7 +89,7 @@ def run_http_activity(activity: Activity, configuration: Configuration, raise ActivityFailed("activity took too long to complete") -def validate_http_activity(activity: Activity): +def validate_http_activity(activity: Activity) -> List[ChaosException]: """ Validate a HTTP activity. @@ -102,15 +102,20 @@ def validate_http_activity(activity: Activity): * `"method"` which is the HTTP verb to use (default to `"GET"`) * `"headers"` which must be a mapping of string to string - In all failing cases, raises :exc:`InvalidActivity`. + In all failing cases, returns a list of errors. This should be considered as a private function. """ + errors = [] + provider = activity["provider"] url = provider.get("url") if not url: - raise InvalidActivity("a HTTP activity must have a URL") + errors.append(InvalidActivity("a HTTP activity must have a URL")) headers = provider.get("headers") if headers and not type(headers) == dict: - raise InvalidActivity("a HTTP activities expect headers as a mapping") + errors.append( + InvalidActivity("a HTTP activities expect headers as a mapping")) + + return errors diff --git a/chaoslib/provider/process.py b/chaoslib/provider/process.py index a463c24..f83a7c8 100644 --- a/chaoslib/provider/process.py +++ b/chaoslib/provider/process.py @@ -4,12 +4,12 @@ import os.path import shutil import subprocess -from typing import Any +from typing import Any, List from logzero import logger from chaoslib import decode_bytes, substitute -from chaoslib.exceptions import ActivityFailed, InvalidActivity +from chaoslib.exceptions import ActivityFailed, InvalidActivity, ChaosException from chaoslib.types import Activity, Configuration, Secrets @@ -67,7 +67,7 @@ def run_process_activity(activity: Activity, configuration: Configuration, } -def validate_process_activity(activity: Activity): +def validate_process_activity(activity: Activity) -> List[ChaosException]: """ Validate a process activity. @@ -76,24 +76,32 @@ def validate_process_activity(activity: Activity): * a `"path"` key which is an absolute path to an executable the current user can call - In all failing cases, raises :exc:`InvalidActivity`. + In all failing cases, returns a list of errors. This should be considered as a private function. """ - name = activity["name"] - provider = activity["provider"] + errors = [] + + name = activity.get("name") + provider = activity.get("provider") path = provider.get("path") if not path: - raise InvalidActivity("a process activity must have a path") + errors.append(InvalidActivity("a process activity must have a path")) + # cannot validate any further, if no path is defined + return errors path = shutil.which(path) if not path: - raise InvalidActivity( + errors.append(InvalidActivity( "path '{path}' cannot be found, in activity '{name}'".format( - path=path, name=name)) + path=path, name=name))) + # cannot validate any further, if path cannot be found + return errors if not os.access(path, os.X_OK): - raise InvalidActivity( + errors.append(InvalidActivity( "no access permission to '{path}', in activity '{name}'".format( - path=path, name=name)) + path=path, name=name))) + + return errors diff --git a/chaoslib/provider/python.py b/chaoslib/provider/python.py index 04ac6ab..f7e6f8a 100644 --- a/chaoslib/provider/python.py +++ b/chaoslib/provider/python.py @@ -3,12 +3,12 @@ import inspect import sys import traceback -from typing import Any +from typing import Any, List from logzero import logger from chaoslib import substitute -from chaoslib.exceptions import ActivityFailed, InvalidActivity +from chaoslib.exceptions import ActivityFailed, InvalidActivity, ChaosException from chaoslib.types import Activity, Configuration, Secrets @@ -60,7 +60,7 @@ def run_python_activity(activity: Activity, configuration: Configuration, sys.exc_info()[2]) -def validate_python_activity(activity: Activity): +def validate_python_activity(activity: Activity) -> List[ChaosException]: """ Validate a Python activity. @@ -72,26 +72,37 @@ def validate_python_activity(activity: Activity): The `"arguments"` activity key must match the function's signature. - In all failing cases, raises :exc:`InvalidActivity`. + In all failing cases, returns a list of errors. This should be considered as a private function. """ - name = activity["name"] - provider = activity["provider"] + errors = [] + + name = activity.get("name") + provider = activity.get("provider") mod_name = provider.get("module") if not mod_name: - raise InvalidActivity("a Python activity must have a module path") + errors.append( + InvalidActivity("a Python activity must have a module path")) func = provider.get("func") if not func: - raise InvalidActivity("a Python activity must have a function name") + errors.append( + InvalidActivity("a Python activity must have a function name")) + + if not mod_name or not func: + # no module no function, we cannot do anymore validation + return errors try: mod = importlib.import_module(mod_name) except ImportError: - raise InvalidActivity("could not find Python module '{mod}' " - "in activity '{name}'".format( - mod=mod_name, name=name)) + errors.append( + InvalidActivity("could not find Python module '{mod}' " + "in activity '{name}'".format( + mod=mod_name, name=name))) + # module is not imported, we cannot do any more validation + return errors found_func = False arguments = provider.get("arguments", {}) @@ -126,21 +137,23 @@ def validate_python_activity(activity: Activity): msg = str(x) if "missing" in msg: arg = msg.rsplit(":", 1)[1].strip() - raise InvalidActivity( + errors.append(InvalidActivity( "required argument {arg} is missing from " - "activity '{name}'".format(arg=arg, name=name)) + "activity '{name}'".format(arg=arg, name=name))) elif "unexpected" in msg: arg = msg.rsplit(" ", 1)[1].strip() - raise InvalidActivity( + errors.append(InvalidActivity( "argument {arg} is not part of the " "function signature in activity '{name}'".format( - arg=arg, name=name)) + arg=arg, name=name))) else: # another error? let's fail fast raise break if not found_func: - raise InvalidActivity( + errors.append(InvalidActivity( "'{mod}' does not expose '{func}' in activity '{name}'".format( - mod=mod_name, func=func, name=name)) + mod=mod_name, func=func, name=name))) + + return errors diff --git a/tests/test_action.py b/tests/test_action.py index e9c118e..f125932 100644 --- a/tests/test_action.py +++ b/tests/test_action.py @@ -12,6 +12,6 @@ def test_empty_action_is_invalid(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(actions.EmptyAction) - assert "empty activity is no activity" in str(exc.value) + errors = ensure_activity_is_valid(actions.EmptyAction) + assert "empty activity is no activity" in str(errors[0]) + diff --git a/tests/test_control.py b/tests/test_control.py index 7e2ce02..49aa553 100644 --- a/tests/test_control.py +++ b/tests/test_control.py @@ -232,13 +232,14 @@ def test_validate_python_control_must_be_loadable(logger): def test_validate_python_control_needs_a_module(): - with pytest.raises(InvalidActivity): - validate_python_control({ + errors = validate_python_control({ "name": "a-python-control", "provider": { "type": "python" } }) + assert len(errors) + assert type(errors[0]) == InvalidActivity def test_controls_can_access_experiment(): diff --git a/tests/test_experiment.py b/tests/test_experiment.py index 2bf9624..4340fef 100644 --- a/tests/test_experiment.py +++ b/tests/test_experiment.py @@ -102,7 +102,7 @@ def test_experiment_hypothesis_must_have_a_title(): def test_experiment_hypothesis_must_have_a_valid_probe(): - with pytest.raises(InvalidActivity) as exc: + with pytest.raises(InvalidExperiment) as exc: ensure_experiment_is_valid(experiments.ExperimentWithInvalidHypoProbe) assert "required argument 'path' is missing from activity" in str(exc.value) diff --git a/tests/test_extension.py b/tests/test_extension.py index d7fa35d..be32284 100644 --- a/tests/test_extension.py +++ b/tests/test_extension.py @@ -9,10 +9,10 @@ def test_extensions_must_have_name(): - with pytest.raises(InvalidExperiment): - exp = experiments.Experiment.copy() - set_extension(exp, {"somekey": "blah"}) - validate_extensions(exp) + exp = experiments.Experiment.copy() + set_extension(exp, {"somekey": "blah"}) + errors = validate_extensions(exp) + assert len(errors) def test_get_extension_returns_nothing_when_not_extensions_block(): diff --git a/tests/test_probe.py b/tests/test_probe.py index a60bff9..300aee3 100644 --- a/tests/test_probe.py +++ b/tests/test_probe.py @@ -14,77 +14,83 @@ from fixtures import config, experiments, probes +def assert_in_errors(msg, errors): + """ + Check whether msg can be found in any of the list of errors + + :param msg: exception string to be found in any of the instances + :param errors: list of ChaosException instances + """ + for error in errors: + if msg in str(error): + # expected exception message is found + return + + raise AssertionError("{} not in {}".format(msg, errors)) + + def test_empty_probe_is_invalid(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.EmptyProbe) - assert "empty activity is no activity" in str(exc.value) + errors = ensure_activity_is_valid(probes.EmptyProbe) + assert_in_errors("empty activity is no activity", errors) def test_probe_must_have_a_type(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.MissingTypeProbe) - assert "an activity must have a type" in str(exc.value) + errors = ensure_activity_is_valid(probes.MissingTypeProbe) + assert_in_errors("an activity must have a type", errors) def test_probe_must_have_a_known_type(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.UnknownTypeProbe) - assert "'whatever' is not a supported activity type" in str(exc.value) + errors = ensure_activity_is_valid(probes.UnknownTypeProbe) + assert_in_errors("'whatever' is not a supported activity type", errors) def test_probe_provider_must_have_a_known_type(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.UnknownProviderTypeProbe) - assert "unknown provider type 'pizza'" in str(exc.value) + errors = ensure_activity_is_valid(probes.UnknownProviderTypeProbe) + assert_in_errors("unknown provider type 'pizza'", errors) def test_python_probe_must_have_a_module_path(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.MissingModuleProbe) - assert "a Python activity must have a module path" in str(exc.value) + errors = ensure_activity_is_valid(probes.MissingModuleProbe) + assert_in_errors("a Python activity must have a module path", errors) def test_python_probe_must_have_a_function_name(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.MissingFunctionProbe) - assert "a Python activity must have a function name" in str(exc.value) + errors = ensure_activity_is_valid(probes.MissingFunctionProbe) + assert_in_errors("a Python activity must have a function name", errors) def test_python_probe_must_be_importable(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.NotImportableModuleProbe) - assert "could not find Python module 'fake.module'" in str(exc.value) + errors = ensure_activity_is_valid(probes.NotImportableModuleProbe) + assert_in_errors("could not find Python module 'fake.module'", errors) def test_python_probe_func_must_have_enough_args(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.MissingFuncArgProbe) - assert "required argument 'path' is missing" in str(exc.value) + errors = ensure_activity_is_valid(probes.MissingFuncArgProbe) + assert_in_errors("required argument 'path' is missing", errors) def test_python_probe_func_cannot_have_too_many_args(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.TooManyFuncArgsProbe) - assert "argument 'should_not_be_here' is not part of the " \ - "function signature" in str(exc.value) + errors = ensure_activity_is_valid(probes.TooManyFuncArgsProbe) + assert_in_errors( + "argument 'should_not_be_here' is not part of the function signature", + errors) def test_process_probe_have_a_path(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.MissingProcessPathProbe) - assert "a process activity must have a path" in str(exc.value) + errors = ensure_activity_is_valid(probes.MissingProcessPathProbe) + assert_in_errors("a process activity must have a path", errors) def test_process_probe_path_must_exist(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.ProcessPathDoesNotExistProbe) - assert "path 'None' cannot be found, in activity" in str(exc.value) + print(probes.ProcessPathDoesNotExistProbe) + errors = ensure_activity_is_valid(probes.ProcessPathDoesNotExistProbe) + print(errors) + assert_in_errors("path 'None' cannot be found, in activity", errors) def test_http_probe_must_have_a_url(): - with pytest.raises(InvalidActivity) as exc: - ensure_activity_is_valid(probes.MissingHTTPUrlProbe) - assert "a HTTP activity must have a URL" in str(exc.value) + errors = ensure_activity_is_valid(probes.MissingHTTPUrlProbe) + assert_in_errors("a HTTP activity must have a URL", errors) def test_run_python_probe_should_return_raw_value(): From 2cacb7b40dbbb3ce8c64f4e77d96f0548202fbb6 Mon Sep 17 00:00:00 2001 From: David Martin Date: Wed, 18 Dec 2019 14:23:52 +0100 Subject: [PATCH 2/2] refactor - use ValitationError exception subclass to handle & format list of errors in message Signed-off-by: David Martin --- chaoslib/exceptions.py | 29 ++++++++++++++++++++++++++++- chaoslib/experiment.py | 14 +++++++------- tests/test_experiment.py | 16 ++++++++-------- 3 files changed, 43 insertions(+), 16 deletions(-) diff --git a/chaoslib/exceptions.py b/chaoslib/exceptions.py index ef4476f..3697c73 100644 --- a/chaoslib/exceptions.py +++ b/chaoslib/exceptions.py @@ -3,7 +3,7 @@ __all__ = ["ChaosException", "InvalidExperiment", "InvalidActivity", "ActivityFailed", "DiscoveryFailed", "InvalidSource", "InterruptExecution", "ControlPythonFunctionLoadingError", - "InvalidControl"] + "InvalidControl", "ValidationError"] class ChaosException(Exception): @@ -44,3 +44,30 @@ class InterruptExecution(ChaosException): class InvalidControl(ChaosException): pass + + +class ValidationError(ChaosException): + def __init__(self, msg, errors, *args, **kwargs): + """ + :param msg: exception message + :param errors: single error as string or list of errors/exceptions + """ + if isinstance(errors, str): + errors = [errors] + self.errors = errors + super().__init__(msg, *args, **kwargs) + + def __str__(self) -> str: + errors = self.errors + nb_errors = len(errors) + err_msg = super().__str__() + return ( + "{msg}{dot} {nb} validation error{plural}:\n" + " - {errors}".format( + msg=err_msg, + dot="" if err_msg.endswith(".") else ".", + nb=nb_errors, + plural="" if nb_errors == 1 else "s", + errors="\n - ".join([str(err) for err in errors]) + ) + ) diff --git a/chaoslib/experiment.py b/chaoslib/experiment.py index 1805eb0..154ad2f 100644 --- a/chaoslib/experiment.py +++ b/chaoslib/experiment.py @@ -15,7 +15,7 @@ cleanup_global_controls from chaoslib.deprecation import warn_about_deprecated_features from chaoslib.exceptions import ActivityFailed, ChaosException, \ - InterruptExecution, InvalidActivity, InvalidExperiment + InterruptExecution, InvalidActivity, InvalidExperiment, ValidationError from chaoslib.extension import validate_extensions from chaoslib.configuration import load_configuration from chaoslib.hypothesis import ensure_hypothesis_is_valid, \ @@ -55,10 +55,14 @@ def ensure_experiment_is_valid(experiment: Experiment): """ logger.info("Validating the experiment's syntax") + full_validation_msg = 'Experiment is not valid, ' \ + 'please fix the following errors' errors = [] if not experiment: - raise InvalidExperiment("an empty experiment is not an experiment") + # empty experiment, cannot continue validation any further + raise ValidationError(full_validation_msg, + "an empty experiment is not an experiment") if not experiment.get("title"): errors.append(InvalidExperiment("experiment requires a title")) @@ -103,11 +107,7 @@ def ensure_experiment_is_valid(experiment: Experiment): errors.extend(validate_controls(experiment)) if errors: - full_validation_msg = 'Experiment is not valid, ' \ - 'please fix the following errors:' - for error in errors: - full_validation_msg += '\n- {}'.format(error) - raise InvalidExperiment(full_validation_msg) + raise ValidationError(full_validation_msg, errors) logger.info("Experiment looks valid") diff --git a/tests/test_experiment.py b/tests/test_experiment.py index 4340fef..a2fa1d9 100644 --- a/tests/test_experiment.py +++ b/tests/test_experiment.py @@ -12,7 +12,7 @@ import yaml from chaoslib.exceptions import ActivityFailed, InvalidActivity, \ - InvalidExperiment, InterruptExecution + InvalidExperiment, InterruptExecution, ValidationError from chaoslib.experiment import ensure_experiment_is_valid, load_experiment, \ run_experiment, run_activities from chaoslib.types import Experiment @@ -21,7 +21,7 @@ def test_empty_experiment_is_invalid(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.EmptyExperiment) assert "an empty experiment is not an experiment" in str(exc.value) @@ -65,27 +65,27 @@ def test_unknown_extension(): def test_experiment_must_have_a_method(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.MissingMethodExperiment) assert "an experiment requires a method with "\ "at least one activity" in str(exc.value) def test_experiment_must_have_at_least_one_step(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.NoStepsMethodExperiment) assert "an experiment requires a method with "\ "at least one activity" in str(exc.value) def test_experiment_must_have_a_title(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.MissingTitleExperiment) assert "experiment requires a title" in str(exc.value) def test_experiment_must_have_a_description(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.MissingDescriptionExperiment) assert "experiment requires a description" in str(exc.value) @@ -96,13 +96,13 @@ def test_experiment_may_not_have_a_hypothesis(): def test_experiment_hypothesis_must_have_a_title(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.MissingHypothesisTitleExperiment) assert "hypothesis requires a title" in str(exc.value) def test_experiment_hypothesis_must_have_a_valid_probe(): - with pytest.raises(InvalidExperiment) as exc: + with pytest.raises(ValidationError) as exc: ensure_experiment_is_valid(experiments.ExperimentWithInvalidHypoProbe) assert "required argument 'path' is missing from activity" in str(exc.value)