diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7a3b1e7f..686985d7 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -44,7 +44,7 @@ jobs: run: python -m tox benchmarks: - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v3 diff --git a/docs/usage.rst b/docs/usage.rst index 1d14a9a5..55d2698c 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -179,7 +179,9 @@ Methods for analysing direct imports .. py:function:: ImportGraph.count_imports() - :return: The number of direct imports in the graph. + :return: The number of imports in the graph. For backward compatibility reasons, ``count_imports`` does not actually + return the number of imports, but the number of dependencies between modules. + So if a module is imported twice from the same module, it will only be counted once. :rtype: Integer. Methods for analysing import chains diff --git a/rust/.cargo/config b/rust/.cargo/config.toml similarity index 100% rename from rust/.cargo/config rename to rust/.cargo/config.toml diff --git a/tests/benchmarking/test_benchmarking.py b/tests/benchmarking/test_benchmarking.py index cbc6b616..061020df 100644 --- a/tests/benchmarking/test_benchmarking.py +++ b/tests/benchmarking/test_benchmarking.py @@ -2,6 +2,7 @@ import json from pathlib import Path from grimp.adaptors.graph import ImportGraph +import grimp @pytest.fixture(scope="module") @@ -17,6 +18,19 @@ def large_graph(): return graph +def test_build_django(benchmark): + """ + Benchmarks building a graph of real package - in this case Django. + """ + fn = lambda: grimp.build_graph("django") + if hasattr(benchmark, "pendantic"): + # Running with pytest-benchmark + benchmark.pedantic(fn, rounds=3) + else: + # Running with codspeed. + benchmark(fn) + + def test_top_level_large_graph(large_graph, benchmark): benchmark( lambda: large_graph.find_illegal_dependencies_for_layers( diff --git a/tests/unit/adaptors/graph/test_chains.py b/tests/unit/adaptors/graph/test_chains.py index 86efb6e7..98965fd4 100644 --- a/tests/unit/adaptors/graph/test_chains.py +++ b/tests/unit/adaptors/graph/test_chains.py @@ -25,13 +25,13 @@ def test_find_downstream_modules(module, as_package, expected_result): graph.add_module(external, is_squashed=True) - graph.add_import(imported=a, importer=b) - graph.add_import(imported=a, importer=c) - graph.add_import(imported=c, importer=d) - graph.add_import(imported=d, importer=e) - graph.add_import(imported=f, importer=b) - graph.add_import(imported=f, importer=g) - graph.add_import(imported=external, importer=d) + graph.add_import(importer=b, imported=a) + graph.add_import(importer=c, imported=a) + graph.add_import(importer=d, imported=c) + graph.add_import(importer=e, imported=d) + graph.add_import(importer=b, imported=f) + graph.add_import(importer=g, imported=f) + graph.add_import(importer=d, imported=external) assert expected_result == graph.find_downstream_modules(module, as_package=as_package) @@ -41,6 +41,7 @@ def test_find_downstream_modules(module, as_package, expected_result): ( ("foo.d", False, {"foo.d.c", "foo.a"}), ("foo.b.g", False, set()), + # Note: foo.d.c is not included in the upstreams because that's internal to the package. ("foo.d", True, {"foo.a", "foo.a.f", "foo.b.g"}), ("foo.b.g", True, set()), ("bar", True, {"foo.a.f", "foo.b.g"}), @@ -56,13 +57,13 @@ def test_find_upstream_modules(module, as_package, expected_result): graph.add_module(external, is_squashed=True) - graph.add_import(imported=a, importer=b) - graph.add_import(imported=a, importer=c) - graph.add_import(imported=c, importer=d) - graph.add_import(imported=d, importer=e) - graph.add_import(imported=f, importer=b) - graph.add_import(imported=g, importer=f) - graph.add_import(imported=f, importer=external) + graph.add_import(importer=b, imported=a) + graph.add_import(importer=c, imported=a) + graph.add_import(importer=d, imported=c) + graph.add_import(importer=e, imported=d) + graph.add_import(importer=b, imported=f) + graph.add_import(importer=f, imported=g) + graph.add_import(importer=external, imported=f) assert expected_result == graph.find_upstream_modules(module, as_package=as_package) @@ -169,6 +170,44 @@ def test_demonstrate_nondeterminism_of_equal_chains(self): class TestFindShortestChains: + @pytest.mark.parametrize( + "importer, imported", + ( + ("green", "green.one"), + ("green.one", "green"), + ), + ) + def test_modules_with_shared_descendants_raises_value_error_when_as_packages_true( + self, importer: str, imported: str + ): + graph = ImportGraph() + graph.add_module(importer) + graph.add_module(imported) + + with pytest.raises(ValueError, match="Modules have shared descendants."): + graph.find_shortest_chains(importer=importer, imported=imported, as_packages=True) + + @pytest.mark.parametrize( + "importer, imported", + ( + ("green", "green.one"), + ("green.one", "green"), + ), + ) + def test_modules_with_shared_descendants_allowed_when_as_packages_false( + self, importer: str, imported: str + ): + graph = ImportGraph() + middle = "middle" + graph.add_import(importer=importer, imported=middle) + graph.add_import(importer=middle, imported=imported) + + result = graph.find_shortest_chains( + importer=importer, imported=imported, as_packages=False + ) + + assert result == {(importer, middle, imported)} + @pytest.mark.parametrize("as_packages", (False, True)) def test_top_level_import(self, as_packages: bool): graph = ImportGraph() @@ -340,6 +379,25 @@ def test_long_indirect_import(self, as_packages: bool, expected_result: Set[Tupl assert result == expected_result + def test_chains_within_packages_are_not_included(self): + graph = ImportGraph() + + graph.add_module("importer_package") + graph.add_module("imported_package") + + # Chain via importer package. + graph.add_import(importer="importer_package.one", imported="importer_package.two") + graph.add_import(importer="importer_package.two", imported="importer_package.three") + graph.add_import(importer="importer_package.three", imported="imported_package.four") + graph.add_import(importer="imported_package.four", imported="imported_package.five") + graph.add_import(importer="imported_package.five", imported="imported_package.six") + + result = graph.find_shortest_chains( + importer="importer_package", imported="imported_package" + ) + + assert result == {("importer_package.three", "imported_package.four")} + def test_chains_via_importer_package_dont_stop_longer_chains_being_included(self): graph = ImportGraph() @@ -501,6 +559,9 @@ def test_doesnt_change_import_count(self, as_packages: bool): # Importer is child of imported (but doesn't import). This doesn't # make sense if as_packages is True, so it should raise an exception. ("b.two", "b", True, ValueError()), + # Importer is child of imported (but doesn't import). This doesn't + # make sense if as_packages is True, so it should raise an exception. + ("b", "b.two", True, ValueError()), # Importer's child imports imported's child (b.two.green -> a.one.green). ("b.two", "a.one", True, True), # Importer's grandchild directly imports imported's grandchild diff --git a/tests/unit/adaptors/graph/test_hierarchy.py b/tests/unit/adaptors/graph/test_hierarchy.py index 5206b541..8053b40e 100644 --- a/tests/unit/adaptors/graph/test_hierarchy.py +++ b/tests/unit/adaptors/graph/test_hierarchy.py @@ -1,6 +1,7 @@ import pytest # type: ignore from grimp.adaptors.graph import ImportGraph +from grimp.exceptions import ModuleNotPresent @pytest.mark.parametrize( @@ -18,6 +19,14 @@ def test_find_children(module, expected_result): assert expected_result == graph.find_children(module) +def test_find_children_raises_exception_for_missing_module(): + graph = ImportGraph() + graph.add_module("foo.a.one") + + with pytest.raises(ModuleNotPresent): + graph.find_children("foo.a") + + def test_find_children_raises_exception_for_squashed_module(): graph = ImportGraph() module = "foo" @@ -28,6 +37,18 @@ def test_find_children_raises_exception_for_squashed_module(): graph.find_children(module) +def test_adding_same_child_module_twice_does_not_corrupt_hierarchy(): + graph = ImportGraph() + graph.add_module("mypackage.blue") + graph.add_module("mypackage.blue.alpha") + graph.add_module("mypackage.blue") # Add for second time. + graph.add_module("mypackage.blue.beta") + + result = graph.find_children("mypackage.blue") + + assert result == {"mypackage.blue.alpha", "mypackage.blue.beta"} + + @pytest.mark.parametrize( "module, expected_result", ( @@ -56,3 +77,41 @@ def test_find_descendants_raises_exception_for_squashed_module(): with pytest.raises(ValueError, match="Cannot find descendants of a squashed module."): graph.find_descendants(module) + + +def test_find_descendants_works_with_gaps(): + graph = ImportGraph() + graph.add_module("mypackage.foo") + # We do not add "mypackage.foo.blue" - there's a gap. + graph.add_module("mypackage.foo.blue.alpha") + graph.add_module("mypackage.foo.blue.alpha.one") + graph.add_module("mypackage.foo.blue.alpha.two") + graph.add_module("mypackage.foo.blue.beta.three") + graph.add_module("mypackage.bar.green.alpha") + + result = graph.find_descendants("mypackage.foo") + + assert result == { + "mypackage.foo.blue.alpha", + "mypackage.foo.blue.alpha.one", + "mypackage.foo.blue.alpha.two", + "mypackage.foo.blue.beta.three", + } + + +def test_find_descendants_works_if_modules_added_in_different_order(): + graph = ImportGraph() + graph.add_module("mypackage.foo") + graph.add_module("mypackage.foo.blue.alpha") + graph.add_module("mypackage.foo.blue.alpha.one") + graph.add_module("mypackage.bar.green.beta") + # Add the middle item in the hierarchy last. + graph.add_module("mypackage.foo.blue") + + result = graph.find_descendants("mypackage.foo") + + assert result == { + "mypackage.foo.blue", + "mypackage.foo.blue.alpha", + "mypackage.foo.blue.alpha.one", + } diff --git a/tests/unit/adaptors/graph/test_manipulation.py b/tests/unit/adaptors/graph/test_manipulation.py index d6710e54..82a882d8 100644 --- a/tests/unit/adaptors/graph/test_manipulation.py +++ b/tests/unit/adaptors/graph/test_manipulation.py @@ -4,30 +4,28 @@ from grimp.exceptions import ModuleNotPresent -def test_add_module(): - graph = ImportGraph() - module = "foo" - - graph.add_module(module) +class TestAddModule: + def test_add_module(self): + graph = ImportGraph() + module = "foo" - assert graph.modules == {module} + graph.add_module(module) + assert graph.modules == {module} -class TestRemoveModule: - def test_removes_module_from_modules(self): + def test_add_module_does_not_add_ancestors_too(self): graph = ImportGraph() - a, b = {"mypackage.blue", "mypackage.green"} + module = "mypackage.foo.bar" - graph.add_module(a) - graph.add_module(b) - graph.add_import(importer=a, imported=b) + graph.add_module(module) + + assert graph.modules == {"mypackage.foo.bar"} - graph.remove_module(b) - assert graph.modules == {a} +class TestRemoveModule: def test_removes_module_removes_import_details_for_imported(self): graph = ImportGraph() - a, b, c = {"mypackage.blue", "mypackage.green", "mypackage.yellow"} + a, b, c = "mypackage.blue", "mypackage.green", "mypackage.yellow" graph.add_import( importer=a, @@ -53,9 +51,20 @@ def test_removes_module_removes_import_details_for_imported(self): } ] + def test_removes_module_from_modules(self): + graph = ImportGraph() + a, b = "mypackage.blue", "mypackage.green" + + graph.add_module(a) + graph.add_module(b) + graph.add_import(importer=a, imported=b) + + graph.remove_module(b) + assert graph.modules == {a} + def test_removes_module_removes_import_details_for_importer(self): graph = ImportGraph() - a, b, c = {"mypackage.blue", "mypackage.green", "mypackage.yellow"} + a, b, c = "mypackage.blue", "mypackage.green", "mypackage.yellow" graph.add_import( importer=b, @@ -83,7 +92,7 @@ def test_removes_module_removes_import_details_for_importer(self): def test_removing_non_existent_module_doesnt_error(self): graph = ImportGraph() - a, b = {"mypackage.blue", "mypackage.green"} + a, b = "mypackage.blue", "mypackage.green" graph.add_module(a) graph.add_module(b) @@ -174,7 +183,7 @@ def test_removes_from_modules(self): def test_removes_from_import_details(self): graph = ImportGraph() - a, b, c = {"mypackage.blue", "mypackage.green", "mypackage.yellow"} + a, b, c = "mypackage.blue", "mypackage.green", "mypackage.yellow" graph.add_import( importer=a, diff --git a/tox.ini b/tox.ini index ab15fed3..2830d425 100644 --- a/tox.ini +++ b/tox.ini @@ -25,7 +25,7 @@ deps = pytest-cov==5.0.0 pytest-benchmark==4.0.0 # External packages to attempt to build the graph from. - django + Django==4.2.17 # N.B. Django 5 doesn't support Python 3.9. flask==3.0.3 requests==2.32.3 sqlalchemy==2.0.35 @@ -59,6 +59,7 @@ deps = pytest==7.4.4 PyYAML==6.0.1 pytest-benchmark==4.0.0 + Django==5.1.1 commands = {posargs:pytest --benchmark-only --benchmark-autosave} @@ -74,6 +75,7 @@ deps = pytest==7.4.4 pyyaml==6.0.1 pytest-codspeed==2.2.1 + Django==5.1.1 commands = {posargs:pytest --codspeed}