From 5e80dd6006135b14273ff68c7c401993d8544efc Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 17 Oct 2018 12:17:10 +0100 Subject: [PATCH 1/2] Corrected layers.yml --- layers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/layers.yml b/layers.yml index 583f61c..076832e 100644 --- a/layers.yml +++ b/layers.yml @@ -3,7 +3,7 @@ Layer Linter architecture: - layer_linter layers: - cmdline - - reports + - report - contract - dependencies - module From 44ca8e669dbca53f1dd09e8ccb8f594ecec3b9b6 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 17 Oct 2018 11:57:37 +0100 Subject: [PATCH 2/2] Error if a layer is missing --- HISTORY.rst | 5 ++ src/layer_linter/cmdline.py | 6 +- src/layer_linter/contract.py | 26 +++++++- src/layer_linter/dependencies/graph.py | 12 +++- tests/unit/dependencies/test_graph.py | 6 ++ tests/unit/test_contract.py | 85 +++++++++++++++++++++++--- 6 files changed, 129 insertions(+), 11 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 882373e..061cc0a 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -102,3 +102,8 @@ History * Improved handling of invalid containers. +latest +------ + +* Error if a layer is missing. + diff --git a/src/layer_linter/cmdline.py b/src/layer_linter/cmdline.py index 253e25f..b7069f8 100644 --- a/src/layer_linter/cmdline.py +++ b/src/layer_linter/cmdline.py @@ -107,7 +107,11 @@ def _main(package_name, config_filename=None, is_debug=False, report = report_class(graph) for contract in contracts: - contract.check_dependencies(graph) + try: + contract.check_dependencies(graph) + except Exception as e: + ConsolePrinter.print_error(str(e)) + return EXIT_STATUS_ERROR report.add_contract(contract) report.output() diff --git a/src/layer_linter/contract.py b/src/layer_linter/contract.py index 14f8d10..f986af4 100644 --- a/src/layer_linter/contract.py +++ b/src/layer_linter/contract.py @@ -35,6 +35,8 @@ def __init__(self, name: str, containers: List[Module], layers: List[Layer], self.whitelisted_paths = whitelisted_paths if whitelisted_paths else [] def check_dependencies(self, dependencies: DependencyGraph) -> None: + self._check_all_layers_exist_for_all_containers(dependencies) + self.illegal_dependencies: List[List[str]] = [] logger.debug('Checking dependencies for contract {}...'.format(self)) @@ -43,6 +45,25 @@ def check_dependencies(self, dependencies: DependencyGraph) -> None: for layer in reversed(self.layers): self._check_layer_does_not_import_downstream(layer, container, dependencies) + def _check_all_layers_exist_for_all_containers(self, dependencies: DependencyGraph) -> None: + """ + Raise a ValueError if we couldn't find any Python files for each layer in all containers. + """ + for container in self.containers: + self._check_all_layers_exist_for_container(container, dependencies) + + def _check_all_layers_exist_for_container(self, + container: Module, + dependencies: DependencyGraph) -> None: + """ + Raise a ValueError if we couldn't find any Python files for each layer. + """ + for layer in self.layers: + layer_module = self._get_layer_module(layer, container) + if layer_module not in dependencies: + raise ValueError(f"Missing layer in container '{container}': " + f"module {layer_module} does not exist.") + def _check_layer_does_not_import_downstream(self, layer: Layer, container: Module, dependencies: DependencyGraph) -> None: @@ -80,13 +101,16 @@ def _get_modules_in_layer( List of modules names within that layer, including the layer module itself. Includes grandchildren and deeper. """ - layer_module = Module("{}.{}".format(container, layer.name)) + layer_module = self._get_layer_module(layer, container) modules = [layer_module] modules.extend( dependencies.get_descendants(layer_module) ) return modules + def _get_layer_module(self, layer: Layer, container: Module) -> Module: + return Module("{}.{}".format(container, layer.name)) + def _get_modules_in_downstream_layers( self, layer: Layer, container: Module, dependencies: DependencyGraph ) -> List[Module]: diff --git a/src/layer_linter/dependencies/graph.py b/src/layer_linter/dependencies/graph.py index 69ff42c..1c9535a 100644 --- a/src/layer_linter/dependencies/graph.py +++ b/src/layer_linter/dependencies/graph.py @@ -1,5 +1,5 @@ import logging -from typing import List, Optional +from typing import List, Optional, Any import networkx # type: ignore from networkx.algorithms import shortest_path # type: ignore @@ -129,3 +129,13 @@ def _remove_paths_from_networkx_graph( def _restore_paths_to_networkx_graph(self, import_paths: List[ImportPath]) -> None: for import_path in import_paths: self._add_path_to_networkx_graph(import_path) + + def __contains__(self, item: Any) -> bool: + """ + Returns whether or not the given item is a Module in the dependency graph. + + Usage: + if module in graph: + ... + """ + return item in self.modules diff --git a/tests/unit/dependencies/test_graph.py b/tests/unit/dependencies/test_graph.py index 62d8a8c..8c608a4 100644 --- a/tests/unit/dependencies/test_graph.py +++ b/tests/unit/dependencies/test_graph.py @@ -114,3 +114,9 @@ def test_dependency_count(self): # Should be number of ImportPaths returned by DependencyAnalyzer.determine_import_paths. assert graph.dependency_count == 3 + + def test_contains(self): + graph = graph_module.DependencyGraph(self.PACKAGE) + + assert Module('foo.one.alpha') in graph + assert Module('foo.one.omega') not in graph diff --git a/tests/unit/test_contract.py b/tests/unit/test_contract.py index 5a910bb..da5587f 100644 --- a/tests/unit/test_contract.py +++ b/tests/unit/test_contract.py @@ -30,15 +30,13 @@ class StubDependencyGraph: A call to .find_path(downstream=downstream_module, upstream=upstream_module) will return the supplied path, if it is present in the dictionary. Otherwise, the stub will return None. + modules: List of all modules in the graph. Usage: graph = StubDependencyGraph( descendants={ Module('foo.one'): [ - Module('foo.one'), Module('foo.one.red'), Module('foo.one.green') - ], - Module('foo.two'): [ - Module('foo.two'), + Module('foo.one.red'), Module('foo.one.green') ], }, paths = { @@ -48,13 +46,15 @@ class StubDependencyGraph: ], }, ... - } + }, + modules [Module('foo.one'), Module('foo.one.red'), Module('foo.one.green')], ) """ - def __init__(self, descendants, paths): - self.descendants = descendants - self.paths = paths + def __init__(self, descendants=None, paths=None, modules=None): + self.descendants = descendants if descendants else {} + self.paths = paths if paths else {} + self.modules = modules if modules else [] def get_descendants(self, module): try: @@ -68,6 +68,9 @@ def find_path(self, downstream, upstream, ignore_paths=None): except KeyError: return None + def __contains__(self, item): + return item in self.modules + class TestContractCheck: def test_kept_contract(self): @@ -120,6 +123,18 @@ def test_kept_contract(self): Module('foo.green.one.alpha')] }, }, + modules=[ + Module('foo.green'), + Module('foo.green.one'), + Module('foo.green.one.alpha'), + Module('foo.green.one.beta'), + Module('foo.green.two'), + Module('foo.green.three'), + Module('foo.blue'), + Module('foo.blue.one'), + Module('foo.blue.two'), + Module('foo.blue.three'), + ] ) contract.check_dependencies(graph) @@ -163,6 +178,18 @@ def test_broken_contract(self): Module('foo.green.three.alpha')], }, }, + modules=[ + Module('foo.green'), + Module('foo.green.one'), + Module('foo.green.one.alpha'), + Module('foo.green.one.beta'), + Module('foo.green.two'), + Module('foo.green.three'), + Module('foo.blue'), + Module('foo.blue.one'), + Module('foo.blue.two'), + Module('foo.blue.three'), + ] ) contract.check_dependencies(graph) @@ -217,6 +244,11 @@ def test_broken_contract_via_other_layer(self): Module('foo.one'): [Module('foo.one'), Module('foo.two')], }, }, + modules=[ + Module('foo.one'), + Module('foo.two'), + Module('foo.three'), + ] ) contract.check_dependencies(graph) @@ -226,6 +258,8 @@ def test_broken_contract_via_other_layer(self): [Module('foo.two'), Module('foo.three')], ] + + @pytest.mark.parametrize('longer_first', (True, False)) def test_only_shortest_violation_is_reported(self, longer_first): contract = Contract( @@ -273,6 +307,7 @@ def test_only_shortest_violation_is_reported(self, longer_first): Module('foo.one.alpha.blue'), Module('foo.one.alpha.green')], }, paths=paths, + modules=[Module('foo.one'), Module('foo.two')] ) contract.check_dependencies(graph) @@ -286,6 +321,40 @@ def test_only_shortest_violation_is_reported(self, longer_first): [Module('foo.one.alpha'), Module('foo.another'), Module('foo.two')], ] + def test_missing_contract_raises_exception(self): + contract = Contract( + name='Foo contract', + containers=( + 'foo.one', + 'foo.two', + ), + layers=( + Layer('blue'), + Layer('yellow'), # Missing from foo.two. + Layer('green'), + ), + ) + graph = StubDependencyGraph( + modules=[ + Module('foo.one'), + Module('foo.one.blue'), + Module('foo.one.blue.alpha'), + Module('foo.one.yellow'), + Module('foo.one.green'), + Module('foo.two'), + Module('foo.two.blue'), + Module('foo.two.blue.alpha'), + Module('foo.two.green'), + ] + ) + + with pytest.raises(ValueError) as e: + contract.check_dependencies(graph) + + assert str(e.value) == ( + "Missing layer in container 'foo.two': module foo.two.yellow does not exist." + ) + @mock.patch.object(importlib.util, 'find_spec') class TestContractFromYAML: