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

@@ -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',

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

pegleg/engine/exceptions.py

@@ -14,12 +14,8 @@
 
 import logging
 
-__all__ = ('PeglegBaseException',
+__all__ = ('PeglegBaseException', 'GitException', 'GitAuthException',
-           'GitException',
+           'GitProxyException', 'GitSSHException', 'GitConfigException',
-           'GitAuthException',
-           'GitProxyException',
-           'GitSSHException',
-           'GitConfigException',
            'GitInvalidRepoException')
 
 LOG = logging.getLogger(__name__)

pegleg/engine/util/definition.py

@@ -63,8 +63,7 @@ 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(
+        raise click.ClickException('failed to get "%s" from site definition '
-            'failed to get "%s" from site definition '
                                    '"%s": %s' % (key, site_name, e))
 
 

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

tests/unit/test_utils.py

@@ -23,8 +23,8 @@ import uuid
 
 _PROXY_SERVERS = {
     'http':
-    os.getenv('HTTP_PROXY',
+    os.getenv('HTTP_PROXY', os.getenv('http_proxy',
-              os.getenv('http_proxy', 'http://proxy.example.com')),
+                                      'http://proxy.example.com')),
     'https':
     os.getenv('HTTPS_PROXY',
               os.getenv('https_proxy', 'https://proxy.example.com'))