From 40da373023c508f216a4246ee04efe5ba4bd05de Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Mon, 29 Oct 2018 19:27:06 +0000 Subject: [PATCH] tests: Increase test coverage for lint checks This patch set expands on the unit test coverage for lint checks in test_selectable_linting which only covers a small subset of the lint checks handled by Pegleg. This logic should be properly tested as linting is fundamental to Pegleg functionality. Change-Id: I6a59295982abd22bba8036827cefd4186b68e2fb --- pegleg/cli.py | 3 +- pegleg/engine/errorcodes.py | 11 ++ pegleg/engine/exceptions.py | 8 +- pegleg/engine/util/definition.py | 5 +- tests/unit/engine/test_selectable_linting.py | 159 +++++++++++++++++-- tests/unit/test_utils.py | 4 +- 6 files changed, 166 insertions(+), 24 deletions(-) diff --git a/pegleg/cli.py b/pegleg/cli.py index c7b10192..6a03a707 100644 --- a/pegleg/cli.py +++ b/pegleg/cli.py @@ -97,8 +97,7 @@ ALLOW_MISSING_SUBSTITUTIONS_OPTION = click.option( type=click.BOOL, default=True, help=("Raise Deckhand exception on missing substition sources. " - "Defaults to True.") -) + "Defaults to True.")) EXCLUDE_LINT_OPTION = click.option( '-x', diff --git a/pegleg/engine/errorcodes.py b/pegleg/engine/errorcodes.py index e4f281ed..08261cbf 100644 --- a/pegleg/engine/errorcodes.py +++ b/pegleg/engine/errorcodes.py @@ -20,3 +20,14 @@ FILE_MISSING_YAML_DOCUMENT_HEADER = 'P006' FILE_CONTAINS_INVALID_YAML = 'P007' DOCUMENT_LAYER_MISMATCH = 'P008' SECRET_NOT_ENCRYPTED_POLICY = 'P009' + +ALL_CODES = ( + SCHEMA_STORAGE_POLICY_MISMATCH_FLAG, + REPOS_MISSING_DIRECTORIES_FLAG, + DECKHAND_DUPLICATE_SCHEMA, + DECKHAND_RENDER_EXCEPTION, + FILE_MISSING_YAML_DOCUMENT_HEADER, + FILE_CONTAINS_INVALID_YAML, + DOCUMENT_LAYER_MISMATCH, + SECRET_NOT_ENCRYPTED_POLICY, +) diff --git a/pegleg/engine/exceptions.py b/pegleg/engine/exceptions.py index 8fc8e0eb..6ad7892c 100644 --- a/pegleg/engine/exceptions.py +++ b/pegleg/engine/exceptions.py @@ -14,12 +14,8 @@ import logging -__all__ = ('PeglegBaseException', - 'GitException', - 'GitAuthException', - 'GitProxyException', - 'GitSSHException', - 'GitConfigException', +__all__ = ('PeglegBaseException', 'GitException', 'GitAuthException', + 'GitProxyException', 'GitSSHException', 'GitConfigException', 'GitInvalidRepoException') LOG = logging.getLogger(__name__) diff --git a/pegleg/engine/util/definition.py b/pegleg/engine/util/definition.py index f28705a4..8ef67f35 100644 --- a/pegleg/engine/util/definition.py +++ b/pegleg/engine/util/definition.py @@ -63,9 +63,8 @@ def pluck(site_definition, key): return site_definition['data'][key] except Exception as e: site_name = site_definition.get('metadata', {}).get('name') - raise click.ClickException( - 'failed to get "%s" from site definition ' - '"%s": %s' % (key, site_name, e)) + raise click.ClickException('failed to get "%s" from site definition ' + '"%s": %s' % (key, site_name, e)) def site_files(site_name): diff --git a/tests/unit/engine/test_selectable_linting.py b/tests/unit/engine/test_selectable_linting.py index fb8c26ca..f1503b42 100644 --- a/tests/unit/engine/test_selectable_linting.py +++ b/tests/unit/engine/test_selectable_linting.py @@ -12,11 +12,17 @@ # See the License for the specific language governing permissions and # limitations under the License. +import os + import click import mock import pytest +from deckhand.engine import layering +from deckhand import errors as dh_errors + from pegleg import config +from pegleg.engine import errorcodes from pegleg.engine import lint from tests.unit.fixtures import create_tmp_deployment_files @@ -28,11 +34,25 @@ For more information, see: https://storyboard.openstack.org/#!/story/2003762 class TestSelectableLinting(object): + def setup(self): + self.site_yaml_path = os.path.join(os.getcwd(), 'site_yamls') + + def _exclude_all(self, except_code): + """Helper to generate list of all error codes to exclude except for + ``except_code`` in order to isolate tests that focus on validating + ``warn_lint``: Just check that the expected code issues a warning and + effectively ignore all other errors. + """ + return [code for code in errorcodes.ALL_CODES if code != except_code] + @mock.patch.object(lint, '_verify_deckhand_render', return_value=[]) @mock.patch.object(lint, '_verify_no_unexpected_files', return_value=[]) - def test_lint_excludes_P001(*args): + def test_lint_excludes_P001(self, *args): + """Test exclude flag for P001 - Document has storagePolicy cleartext + (expected is encrypted) yet its schema is a mandatory encrypted type. + """ exclude_lint = ['P001'] - config.set_site_repo('../pegleg/site_yamls/') + config.set_site_repo(self.site_yaml_path) code_1 = 'X001' msg_1 = 'is a secret, but has unexpected storagePolicy: "cleartext"' @@ -44,26 +64,34 @@ class TestSelectableLinting(object): lint, '_verify_file_contents', return_value=msgs) as mock_methed: with pytest.raises(click.ClickException) as expected_exc: - results = lint.full(False, exclude_lint, []) + lint.full(False, exclude_lint, []) assert msg_1 in expected_exc assert msg_2 in expected_exc @pytest.mark.skip(reason=_SKIP_P003_REASON) @mock.patch.object(lint, '_verify_deckhand_render', return_value=[]) - def test_lint_excludes_P003(*args): + def test_lint_excludes_P003(self, *args): + """Test exclude flag for P003 - All repos contain expected + directories. + """ exclude_lint = ['P003'] with mock.patch.object( lint, '_verify_no_unexpected_files', return_value=[('P003', 'test message')]) as mock_method: - lint.full(False, exclude_lint, []) + result = lint.full(False, exclude_lint, []) mock_method.assert_called() + assert not result # Exclude doesn't return anything. @mock.patch.object(lint, '_verify_deckhand_render', return_value=[]) @mock.patch.object(lint, '_verify_no_unexpected_files', return_value=[]) - def test_lint_warns_P001(*args): + def test_lint_warns_P001(self, *args): + """Test lint flag for P001 - Document has storagePolicy cleartext + (expected is encrypted) yet its schema is a mandatory encrypted type. + """ + exclude_lint = self._exclude_all(except_code='P001') warn_lint = ['P001'] - config.set_site_repo('../pegleg/site_yamls/') + config.set_site_repo(self.site_yaml_path) code_1 = 'X001' msg_1 = 'is a secret, but has unexpected storagePolicy: "cleartext"' @@ -75,20 +103,129 @@ class TestSelectableLinting(object): lint, '_verify_file_contents', return_value=msgs) as mock_methed: with pytest.raises(click.ClickException) as expected_exc: - lint.full(False, [], warn_lint) + lint.full( + False, exclude_lint=exclude_lint, warn_lint=warn_lint) assert msg_1 not in expected_exc assert msg_2 in expected_exc @pytest.mark.skip(reason=_SKIP_P003_REASON) @mock.patch.object(lint, '_verify_deckhand_render', return_value=[]) - def test_lint_warns_P003(*args): + def test_lint_warns_P003(self, *args): + """Test warn flag for P003 - All repos contain expected directories.""" + exclude_lint = self._exclude_all(except_code='P003') warn_lint = ['P003'] - config.set_site_repo('../pegleg/site_yamls/') + config.set_site_repo(self.site_yaml_path) with mock.patch.object(lint, '_verify_no_unexpected_files') as mock_method: - lint.full(False, [], warn_lint) + result = lint.full( + False, exclude_lint=exclude_lint, warn_lint=warn_lint) mock_method.assert_called() + assert len(result) == 1 + assert result[0].startswith(warn_lint[0]) + + @mock.patch('pegleg.engine.util.deckhand.layering', autospec=True) + def test_lint_warns_P004(self, mock_layering): + """Test warn flag for P004 - Duplicate Deckhand DataSchema document + detected. + """ + # Stub out Deckhand render logic. + mock_layering.DocumentLayering.return_value.render.return_value = [] + + exclude_lint = self._exclude_all(except_code='P004') + warn_lint = ['P004'] + config.set_site_repo(self.site_yaml_path) + + documents = { + mock.sentinel.site: [{ + # Create 2 duplicate DataSchema documents. + "schema": "deckhand/DataSchema/v1", + "metadata": { + "name": mock.sentinel.document_name + }, + "data": {} + }] * 2 + } + + with mock.patch( + 'pegleg.engine.util.definition.documents_for_each_site', + autospec=True, + return_value=documents): + result = lint.full( + False, exclude_lint=exclude_lint, warn_lint=warn_lint) + assert len(result) == 1 + assert result[0].startswith(warn_lint[0]) + + @mock.patch('pegleg.engine.util.deckhand.layering', autospec=True) + def test_lint_warns_P005(self, mock_layering): + """Test warn flag for P005 - Deckhand rendering exception.""" + # Make Deckhand render expected exception to trigger error code. + mock_layering.DocumentLayering.return_value.render.side_effect = ( + dh_errors.DeckhandException) + + exclude_lint = self._exclude_all(except_code='P005') + warn_lint = ['P005'] + config.set_site_repo(self.site_yaml_path) + + documents = { + mock.sentinel.site: [{ + "schema": "deckhand/DataSchema/v1", + "metadata": { + "name": mock.sentinel.document_name + }, + "data": {} + }] + } + + with mock.patch( + 'pegleg.engine.util.definition.documents_for_each_site', + autospec=True, + return_value=documents): + result = lint.full( + False, exclude_lint=exclude_lint, warn_lint=warn_lint) + assert len(result) == 1 + assert result[0].startswith(warn_lint[0]) + + def test_lint_warns_P006(self, tmpdir): + """Test warn flag for P006 - YAML file missing document header.""" + + exclude_lint = self._exclude_all(except_code='P006') + warn_lint = ['P006'] + config.set_site_repo(self.site_yaml_path) + + p = tmpdir.mkdir(self.__class__.__name__).join("test.yaml") + p.write("foo: bar") + + with mock.patch( + 'pegleg.engine.util.files.all', + autospec=True, + return_value=[p.strpath]): + result = lint.full( + False, exclude_lint=exclude_lint, warn_lint=warn_lint) + assert len(result) == 1 + assert result[0].startswith(warn_lint[0]) + + def test_lint_warns_P007(self, tmpdir): + """Test warn flag for P007 - YAML file is not valid YAML.""" + + exclude_lint = self._exclude_all(except_code='P007') + warn_lint = ['P007'] + config.set_site_repo(self.site_yaml_path) + + p = tmpdir.mkdir(self.__class__.__name__).join("test.yaml") + # Invalid YAML - will trigger error. + p.write("---\nfoo: bar: baz") + + with mock.patch( + 'pegleg.engine.util.files.all', + autospec=True, + return_value=[p.strpath]): + result = lint.full( + False, exclude_lint=exclude_lint, warn_lint=warn_lint) + assert len(result) == 1 + assert result[0].startswith(warn_lint[0]) + + # TODO(felipemonteiro): Add tests for P008, P009. class TestSelectableLintingHelperFunctions(object): diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 90bcf435..67438683 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -23,8 +23,8 @@ import uuid _PROXY_SERVERS = { 'http': - os.getenv('HTTP_PROXY', - os.getenv('http_proxy', 'http://proxy.example.com')), + os.getenv('HTTP_PROXY', os.getenv('http_proxy', + 'http://proxy.example.com')), 'https': os.getenv('HTTPS_PROXY', os.getenv('https_proxy', 'https://proxy.example.com'))