Merge pull request #168 from seddonym/misc-tweaks

Misc tweaks

.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
 

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

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(

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

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",
+    }

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,

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}