From 25ad717b07ada911a10b582ec8b7f112d01f1654 Mon Sep 17 00:00:00 2001 From: Kevin DeJong Date: Thu, 2 May 2024 10:02:04 -0700 Subject: [PATCH 1/2] Add more testing to increase coverage --- test/unit/module/conditions/test_conditions.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/unit/module/conditions/test_conditions.py b/test/unit/module/conditions/test_conditions.py index dd6c26678b..d93d048811 100644 --- a/test/unit/module/conditions/test_conditions.py +++ b/test/unit/module/conditions/test_conditions.py @@ -289,3 +289,18 @@ def test_test_condition(self): {h_region: "us-east-1", h_environment: "dev"} ) ) + + def test_build_scenerios_on_region_with_condition_dne(self): + """Get condition and test""" + template = decode_str( + """ + Conditions: + IsUsEast1: !Equals [!Ref AWS::Region, "us-east-1"] + """ + )[0] + + cfn = Template("", template) + self.assertListEqual( + list(cfn.conditions.build_scenerios_on_region("IsProd", "us-east-1")), + [True, False], + ) From a4f5f6257113d7d320acc9444d95a356a3c45ba7 Mon Sep 17 00:00:00 2001 From: Kevin DeJong Date: Thu, 2 May 2024 11:44:09 -0700 Subject: [PATCH 2/2] Create fixes and more tests --- src/cfnlint/config.py | 94 +++++++--------- src/cfnlint/context/context.py | 10 +- .../module/config/test_config_file_args.py | 48 ++++++++ test/unit/module/config/test_template_args.py | 12 ++ test/unit/module/context/test_conditions.py | 13 +++ .../module/context/test_create_context.py | 104 ++++++++++++++++++ test/unit/module/context/test_mappings.py | 26 +++++ test/unit/module/context/test_parameter.py | 7 +- test/unit/module/context/test_resource.py | 40 +++++++ 9 files changed, 299 insertions(+), 55 deletions(-) create mode 100644 test/unit/module/context/test_conditions.py create mode 100644 test/unit/module/context/test_create_context.py create mode 100644 test/unit/module/context/test_mappings.py create mode 100644 test/unit/module/context/test_resource.py diff --git a/src/cfnlint/config.py b/src/cfnlint/config.py index c034285475..afd51cb221 100644 --- a/src/cfnlint/config.py +++ b/src/cfnlint/config.py @@ -57,9 +57,9 @@ class ConfigFileArgs: """ file_args: Dict = {} - __user_config_file = None - __project_config_file = None - __custom_config_file = None + _user_config_file = None + _project_config_file = None + _custom_config_file = None def __init__(self, schema=None, config_file=None): # self.file_args = self.get_config_file_defaults() @@ -72,10 +72,10 @@ def __init__(self, schema=None, config_file=None): self.schema = self.default_schema if not schema else schema if config_file: - self.__custom_config_file = config_file + self._custom_config_file = config_file else: LOGGER.debug("Looking for CFLINTRC before attempting to load") - self.__user_config_file, self.__project_config_file = self._find_config() + self._user_config_file, self._project_config_file = self._find_config() self.load() @@ -91,24 +91,28 @@ def _find_config(self): > user_config, project_config = self._find_config() """ config_file_name = ".cfnlintrc" - self.__user_config_file = Path.home().joinpath(config_file_name) - - self.__project_config_file = Path.cwd().joinpath(config_file_name) - if self._has_file(config_file_name + ".yaml"): - self.__project_config_file = Path.cwd().joinpath(config_file_name + ".yaml") - elif self._has_file(config_file_name + ".yml"): - self.__project_config_file = Path.cwd().joinpath(config_file_name + ".yml") user_config_path = "" - project_config_path = "" + home_path = Path.home() + for path in [ + home_path.joinpath(config_file_name), + home_path.joinpath(f"{config_file_name}.yaml"), + home_path.joinpath(f"{config_file_name}.yml"), + ]: + if self._has_file(path): + user_config_path = path + break - if self._has_file(self.__user_config_file): - LOGGER.debug("Found User CFNLINTRC") - user_config_path = self.__user_config_file - - if self._has_file(self.__project_config_file): - LOGGER.debug("Found Project level CFNLINTRC") - project_config_path = self.__project_config_file + project_config_path = "" + cwd_path = Path.cwd() + for path in [ + cwd_path.joinpath(config_file_name), + cwd_path.joinpath(f"{config_file_name}.yaml"), + cwd_path.joinpath(f"{config_file_name}.yml"), + ]: + if self._has_file(path): + project_config_path = path + break return user_config_path, project_config_path @@ -133,8 +137,8 @@ def load(self): CFLINTRC configuration """ - if self.__custom_config_file: - custom_config = self._read_config(self.__custom_config_file) + if self._custom_config_file: + custom_config = self._read_config(self._custom_config_file) LOGGER.debug("Validating Custom CFNLINTRC") self.validate_config(custom_config, self.schema) LOGGER.debug("Custom configuration loaded as") @@ -142,11 +146,11 @@ def load(self): self.file_args = custom_config else: - user_config = self._read_config(self.__user_config_file) + user_config = self._read_config(self._user_config_file) LOGGER.debug("Validating User CFNLINTRC") self.validate_config(user_config, self.schema) - project_config = self._read_config(self.__project_config_file) + project_config = self._read_config(self._project_config_file) LOGGER.debug("Validating Project CFNLINTRC") self.validate_config(project_config, self.schema) @@ -577,34 +581,20 @@ def set_template_args(self, template): configs = template.get("Metadata", {}).get("cfn-lint", {}).get("config", {}) if isinstance(configs, dict): - for config_name, config_value in configs.items(): - if config_name == "ignore_checks": - if isinstance(config_value, list): - defaults["ignore_checks"] = config_value - if config_name == "regions": - if isinstance(config_value, list): - defaults["regions"] = config_value - if config_name == "append_rules": - if isinstance(config_value, list): - defaults["append_rules"] = config_value - if config_name == "override_spec": - if isinstance(config_value, (str)): - defaults["override_spec"] = config_value - if config_name == "custom_rules": - if isinstance(config_value, (str)): - defaults["custom_rules"] = config_value - if config_name == "ignore_bad_template": - if isinstance(config_value, bool): - defaults["ignore_bad_template"] = config_value - if config_name == "include_checks": - if isinstance(config_value, list): - defaults["include_checks"] = config_value - if config_name == "configure_rules": - if isinstance(config_value, dict): - defaults["configure_rules"] = config_value - if config_name == "include_experimental": - if isinstance(config_value, bool): - defaults["include_experimental"] = config_value + for key, value in { + "ignore_checks": (list), + "regions": (list), + "append_rules": (list), + "override_spec": (str), + "custom_rules": (str), + "ignore_bad_template": (bool), + "include_checks": (list), + "configure_rules": (dict), + "include_experimental": (bool), + }.items(): + if key in configs: + if isinstance(configs[key], value): + defaults[key] = configs[key] self._template_args = defaults diff --git a/src/cfnlint/context/context.py b/src/cfnlint/context/context.py index 57e81bb93e..9872c8ca8c 100644 --- a/src/cfnlint/context/context.py +++ b/src/cfnlint/context/context.py @@ -224,11 +224,13 @@ class Parameter(_Ref): type: str = field(init=False) default: Any = field(init=False) allowed_values: Any = field(init=False) - description: str = field(init=False) + description: str | None = field(init=False) parameter: InitVar[Any] def __post_init__(self, parameter) -> None: + if not isinstance(parameter, dict): + raise ValueError("Parameter must be a object") self.default = None self.allowed_values = [] self.min_value = None @@ -249,7 +251,7 @@ def __post_init__(self, parameter) -> None: if self.type == "CommaDelimitedList" or self.type.startswith("List<"): if "Default" in parameter: - self.default = parameter.get("Default").split(",") + self.default = parameter.get("Default", "").split(",") for allowed_value in parameter.get("AllowedValues", []): self.allowed_values.append(allowed_value.split(",")) else: @@ -294,6 +296,8 @@ class Resource(_Ref): resource: InitVar[Any] def __post_init__(self, resource) -> None: + if not isinstance(resource, dict): + raise ValueError("Resource must be a object") t = resource.get("Type") if not isinstance(t, str): raise ValueError("Type must be a string") @@ -325,6 +329,8 @@ def __post_init__(self, instance) -> None: for k, v in instance.items(): if isinstance(v, (str, list, int, float)): self.keys[k] = v + else: + raise ValueError("Third keys must not be an object") def value(self, secondary_key: str): if secondary_key not in self.keys: diff --git a/test/unit/module/config/test_config_file_args.py b/test/unit/module/config/test_config_file_args.py index 0858c80c49..86144086c7 100644 --- a/test/unit/module/config/test_config_file_args.py +++ b/test/unit/module/config/test_config_file_args.py @@ -87,3 +87,51 @@ def test_config_parser_fail_on_config_rules(self, yaml_mock): self.assertEqual( results.file_args, {"configure_rules": {"E3012": {"strict": False}}} ) + + @patch("pathlib.Path.is_file", create=True) + def test_config_parser_is_file_both(self, is_file_mock): + calls = [ + True, + True, + False, + False, + ] + is_file_mock.side_effect = calls + my_config = cfnlint.config.ConfigFileArgs() + self.assertEqual(my_config._user_config_file.name, ".cfnlintrc") + self.assertEqual(my_config._project_config_file.name, ".cfnlintrc") + self.assertEqual(is_file_mock.call_count, len(calls)) + + @patch("pathlib.Path.is_file", create=True) + def test_config_parser_is_file_both_yaml(self, is_file_mock): + calls = [ + False, + True, + False, + True, + False, + False, + ] + is_file_mock.side_effect = calls + my_config = cfnlint.config.ConfigFileArgs() + self.assertEqual(my_config._user_config_file.name, ".cfnlintrc.yaml") + self.assertEqual(my_config._project_config_file.name, ".cfnlintrc.yaml") + self.assertEqual(is_file_mock.call_count, len(calls)) + + @patch("pathlib.Path.is_file", create=True) + def test_config_parser_is_file_both_yml(self, is_file_mock): + calls = [ + False, + False, + True, + False, + False, + True, + False, + False, + ] + is_file_mock.side_effect = calls + my_config = cfnlint.config.ConfigFileArgs() + self.assertEqual(my_config._user_config_file.name, ".cfnlintrc.yml") + self.assertEqual(my_config._project_config_file.name, ".cfnlintrc.yml") + self.assertEqual(is_file_mock.call_count, len(calls)) diff --git a/test/unit/module/config/test_template_args.py b/test/unit/module/config/test_template_args.py index bbb4c6c282..08781e6b6b 100644 --- a/test/unit/module/config/test_template_args.py +++ b/test/unit/module/config/test_template_args.py @@ -91,3 +91,15 @@ def test_template_args_failure_good_and_bad_value(self): ) self.assertEqual(config.template_args.get("configure_rules"), None) + + def test_bad_template_structure(self): + """test template args""" + config = cfnlint.config.TemplateArgs([]) + + self.assertEqual(config._template_args, {}) + + def test_bad_config_structure(self): + """test template args""" + config = cfnlint.config.TemplateArgs({"Metadata": {"cfn-lint": {"config": []}}}) + + self.assertEqual(config._template_args, {}) diff --git a/test/unit/module/context/test_conditions.py b/test/unit/module/context/test_conditions.py new file mode 100644 index 0000000000..d499926496 --- /dev/null +++ b/test/unit/module/context/test_conditions.py @@ -0,0 +1,13 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" + +import pytest + +from cfnlint.context.context import _init_conditions + + +def test_conditions(): + with pytest.raises(ValueError): + _init_conditions([]) diff --git a/test/unit/module/context/test_create_context.py b/test/unit/module/context/test_create_context.py new file mode 100644 index 0000000000..26f0c7f22c --- /dev/null +++ b/test/unit/module/context/test_create_context.py @@ -0,0 +1,104 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" + +from collections import namedtuple + +import pytest + +from cfnlint import Template +from cfnlint.context.context import create_context_for_template + +_Counts = namedtuple("_Counts", ["resources", "parameters", "conditions", "mappings"]) + + +@pytest.mark.parametrize( + "name,instance,counts", + [ + ( + "Valid template", + { + "Parameters": { + "Env": { + "Type": "String", + } + }, + "Conditions": { + "IsUsEast1": {"Fn::Equals": [{"Ref": "AWS::Region"}, "us-east-1"]} + }, + "Mappings": {"Map": {"us-east-1": {"foo": "bar"}}}, + "Resources": { + "Bucket": { + "Type": "AWS::S3::Bucket", + }, + }, + }, + _Counts(resources=1, parameters=1, conditions=1, mappings=1), + ), + ( + "Bad types in template", + { + "Parameters": [], + "Conditions": [], + "Mappings": [], + "Resources": [], + }, + _Counts(resources=0, parameters=0, conditions=0, mappings=0), + ), + ( + "Invalid type configurations", + { + "Parameters": { + "BusinessUnit": [], + "Env": { + "Type": "String", + }, + }, + "Mappings": {"AnotherMap": [], "Map": {"us-east-1": {"foo": "bar"}}}, + "Resources": { + "Instance": [], + "Bucket": { + "Type": "AWS::S3::Bucket", + }, + }, + }, + _Counts(resources=1, parameters=1, conditions=0, mappings=1), + ), + ( + "Invalid mapping second key", + { + "Mappings": { + "BadKey": { + "Foo": [], + }, + "Map": {"us-east-1": {"foo": "bar"}}, + }, + }, + _Counts(resources=0, parameters=0, conditions=0, mappings=1), + ), + ( + "Invalid mapping third key", + { + "Mappings": { + "BadKey": { + "Foo": { + "Bar": {}, + }, + }, + "Map": {"us-east-1": {"foo": "bar"}}, + }, + }, + _Counts(resources=0, parameters=0, conditions=0, mappings=1), + ), + ], +) +def test_create_context(name, instance, counts): + cfn = Template("", instance, ["us-east-1"]) + context = create_context_for_template(cfn) + + for i in counts._fields: + assert len(getattr(context, i)) == getattr(counts, i), ( + f"Test {name} has {i} {len(getattr(context, i))} " + "and expected {getattr(counts, i)}" + ) diff --git a/test/unit/module/context/test_mappings.py b/test/unit/module/context/test_mappings.py new file mode 100644 index 0000000000..14b36cff6c --- /dev/null +++ b/test/unit/module/context/test_mappings.py @@ -0,0 +1,26 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" + +import pytest + +from cfnlint.context.context import Map + + +@pytest.mark.parametrize( + "name,get_key_1,get_key_2,expected", + [ + ("Valid get", "A", "B", ["C"]), + ("Invalid first key", "B", "B", KeyError()), + ("Invalid second key", "A", "C", KeyError()), + ], +) +def test_mapping_value(name, get_key_1, get_key_2, expected): + mapping = Map({"A": {"B": "C"}}) + + if isinstance(expected, Exception): + with pytest.raises(type(expected)): + list(mapping.find_in_map(get_key_1, get_key_2)) + else: + assert list(mapping.find_in_map(get_key_1, get_key_2)) == expected diff --git a/test/unit/module/context/test_parameter.py b/test/unit/module/context/test_parameter.py index 0ad51d61f5..24781a4473 100644 --- a/test/unit/module/context/test_parameter.py +++ b/test/unit/module/context/test_parameter.py @@ -8,7 +8,7 @@ import pytest from cfnlint.context import Context -from cfnlint.context.context import Parameter +from cfnlint.context.context import Parameter, _init_parameters @pytest.mark.parametrize( @@ -118,3 +118,8 @@ def test_no_echo(name, instance, expected): def test_errors(name, instance): with pytest.raises(ValueError): Parameter(instance) + + +def test_parameters(): + with pytest.raises(ValueError): + _init_parameters([]) diff --git a/test/unit/module/context/test_resource.py b/test/unit/module/context/test_resource.py new file mode 100644 index 0000000000..582b054e43 --- /dev/null +++ b/test/unit/module/context/test_resource.py @@ -0,0 +1,40 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" + +import pytest + +from cfnlint.context import Context +from cfnlint.context.context import Resource, _init_resources + + +@pytest.mark.parametrize( + "name,instance,expected_ref", + [ + ("Valid resource", {"Type": "AWS::S3::Bucket"}, []), + ], +) +def test_resource(name, instance, expected_ref): + context = Context(["us-east-1"]) + parameter = Resource(instance) + + assert expected_ref == list( + parameter.ref(context) + ), f"{name!r} test got {list(parameter.ref(context))}" + + +@pytest.mark.parametrize( + "name,instance", + [ + ("Invalid Type", {"Type": {}}), + ], +) +def test_errors(name, instance): + with pytest.raises(ValueError): + Resource(instance) + + +def test_resources(): + with pytest.raises(ValueError): + _init_resources([])