Repository: zyskarch/pytestarch (language: python)

Issue:
Help with understanding how to use pytestarch
Hello, there :) 

First of all, just wanted to say thanks for all the effort put into building this project, this is much appreciated!

Reading the documentation and trying it on a real project, I'm struggling to understand how to make it work with pytest.

This is the file structure I have in place: 
```
./app
./app/jobs
./app/jobs/email_processing.py
./app/repositories
./app/repositories/db
./app/repositories/db/account_repository.py
```

Looking into the codebase, `email_processing.py` has an import pointing to `account_repository.py`: `from app.repositories.db.account_repository import AccountRepository`

Then I created a test file called `test_architectural_rules.py` to assert that dependencies in the opposite direction (repository -> jobs) should be forbidden, which looks like below:
```
import os

from pytestarch import get_evaluable_architecture, Rule

evaluable = get_evaluable_architecture('<project root>', '<project root>/app')


def test_that_repositories_dont_depend_on_blueprints():
    rule = (
        Rule()
        .modules_that()
        .are_named("app.jobs.email_processing")
        .should_not()
        .be_imported_by_modules_that()
        .are_sub_modules_of("app.repositories.db")
    )

    rule.assert_applies(evaluable)
```

Assuming I understood the documentation correctly, in theory this should work. But instead I'm getting the error below. What am I doing wrong here? :( 

This is the stacktrace I get when running the test: 
```
test_architectural_rules.py:9 (test_that_repositories_dont_depend_on_blueprints)
self = <networkx.classes.digraph.DiGraph object at 0x111b49fa0>
n = 'app.jobs.email_processing'

    def successors(self, n):
        """Returns an iterator over successor nodes of n.
    
        A successor of n is a node m such that there exists a directed
        edge from n to m.
    
        Parameters
        ----------
        n : node
           A node in the graph
    
        Raises
        ------
        NetworkXError
           If n is not in the graph.
    
        See Also
        --------
        predecessors
    
        Notes
        -----
        neighbors() and successors() are the same.
        """
        try:
>           return iter(self._succ[n])
E           KeyError: 'app.jobs.email_processing'

../../venv/lib/python3.9/site-packages/networkx/classes/digraph.py:899: KeyError

The above exception was the direct cause of the following exception:

    def test_that_repositories_dont_depend_on_blueprints():
        rule = (
            Rule()
            .modules_that()
            .are_named("app.jobs.email_processing")
            .should_not()
            .be_imported_by_modules_that()
            .are_sub_modules_of("app.services")
        )
    
>       rule.assert_applies(evaluable)

test_architectural_rules.py:20: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../venv/lib/python3.9/site-packages/pytestarch/query_language/rule.py:209: in assert_applies
    matcher.match(evaluable)
../../venv/lib/python3.9/site-packages/pytestarch/rule_assessment/rule_check/rule_matcher.py:57: in match
    rule_violations = self._find_rule_violations(evaluable)
../../venv/lib/python3.9/site-packages/pytestarch/rule_assessment/rule_check/rule_matcher.py:64: in _find_rule_violations
    explicitly_requested_dependencies = self._get_explicitly_requested_dependencies(
../../venv/lib/python3.9/site-packages/pytestarch/rule_assessment/rule_check/rule_matcher.py:130: in _get_explicitly_requested_dependencies
    return evaluable.get_dependencies(
../../venv/lib/python3.9/site-packages/pytestarch/eval_structure/evaluable_graph.py:43: in get_dependencies
    dependency = get_dependency_between_modules(
../../venv/lib/python3.9/site-packages/pytestarch/eval_structure/breadth_first_searches.py:14: in get_dependency_between_modules
    dependent_upon_nodes = get_all_submodules_of(graph, dependent_upon)
../../venv/lib/python3.9/site-packages/pytestarch/eval_structure/breadth_first_searches.py:205: in get_all_submodules_of
    children = graph.direct_successor_nodes(node)
../../venv/lib/python3.9/site-packages/pytestarch/eval_structure/networkxgraph.py:193: in direct_successor_nodes
    return sorted(self._graph.successors(node))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <networkx.classes.digraph.DiGraph object at 0x111b49fa0>
n = 'app.jobs.email_processing'

    def successors(self, n):
        """Returns an iterator over successor nodes of n.
    
        A successor of n is a node m such that there exists a directed
        edge from n to m.
    
        Parameters
        ----------
        n : node
           A node in the graph
    
        Raises
        ------
        NetworkXError
           If n is not in the graph.
    
        See Also
        --------
        predecessors
    
        Notes
        -----
        neighbors() and successors() are the same.
        """
        try:
            return iter(self._succ[n])
        except KeyError as err:
>           raise NetworkXError(f"The node {n} is not in the digraph.") from err
E           networkx.exception.NetworkXError: The node app.jobs.email_processing is not in the digraph.

../../venv/lib/python3.9/site-packages/networkx/classes/digraph.py:901: NetworkXError
```

Thanks a lot for any help!


Discussion / hints:
Hi @felipecao, 

The module names are supposed to include the root module (this limitation is needed to be able to separate between internal and external modules), so for example:
If your `app` module's parent module is  '/some/path/root', all modules should start with 'root': "root.app.jobs.email_processing" and so on.

So in your case, there are two options to make it work:
1. Rename by adding "root." to your module names
2. Modify the evaluable to: `get_evaluable_architecture('<project root/app>', '<project root>/app')`

I hope this helps :) 
I will adapt the Readme to make this more obvious, thank you for bringing this up!
Hi @zyskarch ! Thanks a lot for the feedback! 

Thanks to your tip, I was able to get rid of the exception saying that the node is not in the digraph. But I still have the feeling I'm not understanding something. Let me give you another example...

I have the following file structure: 
``` 
./app
./app/blueprints
./app/blueprints/account.py
./app/repositories
./app/repositories/db
./app/repositories/db/gmail_email_account_repository.py
``` 

`account.py` contains the following import statement: `from app.repositories.db.gmail_email_account_repository import GmailEmailAccountRepository`

Now I have the following architectural test, which I was expecting to **fail**, but instead, it passes:
``` 
def test_that_repositories_dont_depend_on_blueprints():
    rule = (
        Rule()
        .modules_that()
        .are_named("root.app.repositories.db.gmail_email_account_repository")
        .should_not()
        .be_imported_by_modules_that()
        .are_sub_modules_of("root.app.blueprints")
    )

    rule.assert_applies(evaluable)
``` 

Did I get it wrong? Wasn't it supposed to fail? How else should i have written this test to make sure it fails?

Thanks a lot!

Fix patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 3e045cd..99def4d 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -2,6 +2,10 @@
 
 This project uses semantic versioning and follows [keep a changelog](https://keepachangelog.com).
 
+## [2.0.2] -- 2024-03-16
+### Fixed
+- Module resolution for absolute imports when root and module path differ.
+
 ## [2.0.1] -- 2024-03-14
 ### Fixed
 - Readme description on module name references.
diff --git a/pyproject.toml b/pyproject.toml
index 061d338..c4c17d2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "PyTestArch"
-version = "2.0.1"
+version = "2.0.2"
 description = "Test framework for software architecture based on imports between modules"
 authors = ["zyskarch <zyskarch@gmail.com>"]
 maintainers = ["zyskarch <zyskarch@gmail.com>"]


Test patch:
diff --git a/src/pytestarch/eval_structure_generation/file_import/converter.py b/src/pytestarch/eval_structure_generation/file_import/converter.py
index c240117..ed5b3fa 100644
--- a/src/pytestarch/eval_structure_generation/file_import/converter.py
+++ b/src/pytestarch/eval_structure_generation/file_import/converter.py
@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 import ast
-from typing import List, Optional, Sequence
+from typing import List, Optional, Sequence, Set
 
 from pytestarch.eval_structure.types import Import
 from pytestarch.eval_structure_generation.file_import.import_types import (
@@ -14,7 +14,13 @@
 class ImportConverter:
     """Converts all ast imports to custom import types."""
 
-    def convert(self, asts: List[NamedModule]) -> Sequence[Import]:
+    def convert(
+        self,
+        asts: List[NamedModule],
+        root_prefix_relevant: bool,
+        root_prefix: str,
+        all_modules: Set[str],
+    ) -> Sequence[Import]:
         """Converts ast modules to custom import modules. Filters out all modules
         that are not imports.
 
@@ -36,7 +42,13 @@ def convert(self, asts: List[NamedModule]) -> Sequence[Import]:
                     [NamedModule(m, module_name) for m in ast_module.body]  # type: ignore
                 )
             else:
-                new_imports = self._convert(module.module, module.name)
+                new_imports = self._convert(
+                    module.module,
+                    module.name,
+                    root_prefix_relevant,
+                    root_prefix,
+                    all_modules,
+                )
 
                 if new_imports:
                     imports.extend(new_imports)
@@ -44,7 +56,12 @@ def convert(self, asts: List[NamedModule]) -> Sequence[Import]:
         return imports
 
     def _convert(
-        self, module: ast.Module, module_name: str
+        self,
+        module: ast.Module,
+        module_name: str,
+        root_prefix_relevant: bool,
+        root_prefix: str,
+        all_internal_modules: Set[str],
     ) -> Optional[Sequence[Import]]:
         """Calculates all imports of the given ast module.
 
@@ -60,10 +77,10 @@ def _convert(
         if isinstance(module, ast.Import):
             new_imports = []
             for alias in module.names:
-                new_imports.append(AbsoluteImport(module_name, alias.name))  # type: ignore
+                new_imports.append(AbsoluteImport(module_name, self._adjust_with_root_prefix(alias.name, root_prefix_relevant, root_prefix, all_internal_modules)))  # type: ignore
         elif isinstance(module, ast.ImportFrom):
             if module.level == 0:
-                new_imports = [AbsoluteImport(module_name, module.module)]  # type: ignore
+                new_imports = [AbsoluteImport(module_name, self._adjust_with_root_prefix(module.module, root_prefix_relevant, root_prefix, all_internal_modules))]  # type: ignore
             else:
                 new_imports = []
                 for alias in module.names:
@@ -74,3 +91,28 @@ def _convert(
                     )
 
         return new_imports
+
+    @classmethod
+    def _adjust_with_root_prefix(
+        cls,
+        module_name: str,
+        root_prefix_relevant: bool,
+        root_prefix: str,
+        all_internal_modules: Set[str],
+    ) -> str:
+        """If the user specifies a module import start point that is not the root path, the ast module names will not contain
+        the root path. Hence, they will later be flagged as external modules.
+        To avoid this, we prepend the root path and check whether this gives a module that we have detected as an internal module earlier.
+        If so, we optimistically assume that this prepended module name is the correct module name.
+
+        Only absolute imports are affected by this, as the relative imports are resolved internally anyway.
+        """
+        if not root_prefix_relevant:
+            return module_name
+
+        potential_full_name = f"{root_prefix}.{module_name}"
+
+        if potential_full_name in all_internal_modules:
+            return potential_full_name
+
+        return module_name
diff --git a/src/pytestarch/eval_structure_generation/graph_generation/graph_generator.py b/src/pytestarch/eval_structure_generation/graph_generation/graph_generator.py
index a2af758..070e289 100644
--- a/src/pytestarch/eval_structure_generation/graph_generation/graph_generator.py
+++ b/src/pytestarch/eval_structure_generation/graph_generation/graph_generator.py
@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 from pathlib import Path
-from typing import List, Optional, Sequence, Tuple
+from typing import List, Optional, Sequence, Set, Tuple
 
 from pytestarch.eval_structure.evaluable_graph import EvaluableArchitectureGraph
 from pytestarch.eval_structure.networkxgraph import NetworkxGraph, Node
@@ -31,12 +31,18 @@ def generate_graph(
     )
 
     all_modules, ast = _get_all_ast_modules(module_path, root_path, exclusions)
-    imports = _get_imports_from_ast(ast)
 
     internal_module_prefix = _get_internal_module_prefix(
         path_diff_between_root_and_module, root_path
     )
 
+    imports = _get_imports_from_ast(
+        ast,
+        _actual_difference_between_root_and_module(path_diff_between_root_and_module),
+        root_path.name,
+        _get_all_internal_modules(all_modules, internal_module_prefix),
+    )
+
     imports = _remove_excluded_imports(
         exclude_external_libraries, imports, internal_module_prefix
     )
@@ -78,16 +84,22 @@ def _get_internal_module_prefix(
 ) -> str:
     """In general, all internal modules should start with the root module name to be able to differentiate between
     internal and external modules. If the root and base module differ, the modules between them also need to be taken
-    into account, as not root.a.b modules are external, but root.a.b.base are internal.
+    into account, as not-root.a.b-modules are external, but root.a.b.base-modules are internal.
     """
     internal_module_prefix = root_path.name + "."
 
-    if path_diff_between_root_and_module != ".":
+    if _actual_difference_between_root_and_module(path_diff_between_root_and_module):
         internal_module_prefix += path_diff_between_root_and_module
 
     return internal_module_prefix
 
 
+def _actual_difference_between_root_and_module(
+    path_diff_between_root_and_module: str,
+) -> bool:
+    return path_diff_between_root_and_module != "."
+
+
 def _add_extra_levels_to_limit_if_root_and_module_path_differ(
     level_limit: Optional[int], path_diff_between_root_and_module: str
 ) -> Optional[int]:
@@ -97,16 +109,23 @@ def _add_extra_levels_to_limit_if_root_and_module_path_differ(
     if level_limit is None:
         return None
 
-    if path_diff_between_root_and_module != ".":
+    if _actual_difference_between_root_and_module(path_diff_between_root_and_module):
         levels = len(path_diff_between_root_and_module.split("."))
         level_limit += levels
 
     return level_limit
 
 
-def _get_imports_from_ast(ast: List[NamedModule]) -> Sequence[Import]:
+def _get_imports_from_ast(
+    ast: List[NamedModule],
+    root_prefix_relevant: bool,
+    root_prefix: str,
+    all_internal_modules: Set[str],
+) -> Sequence[Import]:
     converter = ImportConverter()
-    return converter.convert(ast)
+    return converter.convert(
+        ast, root_prefix_relevant, root_prefix, all_internal_modules
+    )
 
 
 def _get_all_ast_modules(
@@ -118,3 +137,9 @@ def _get_all_ast_modules(
     parser = Parser(file_filter, root_path)
     all_modules, ast = parser.parse(module_path)
     return all_modules, ast
+
+
+def _get_all_internal_modules(
+    modules: List[str], internal_module_prefix: str
+) -> Set[str]:
+    return {m for m in modules if m.startswith(internal_module_prefix)}
diff --git a/tests/eval_structure_generation/test_converter.py b/tests/eval_structure_generation/test_converter.py
index 3e8e1e8..84542ba 100644
--- a/tests/eval_structure_generation/test_converter.py
+++ b/tests/eval_structure_generation/test_converter.py
@@ -16,7 +16,7 @@ def test_converter_retains_all_non_empty_modules() -> None:
     all_modules, asts = parser.parse(RESOURCES_DIR)
 
     converter = ImportConverter()
-    imports = converter.convert(asts)
+    imports = converter.convert(asts, False, "", set())
 
     expected_imports = {
         "pytestarch.pytestarch",
diff --git a/tests/eval_structure_generation/test_module_graph.py b/tests/eval_structure_generation/test_module_graph.py
index 7c4546a..17d422f 100644
--- a/tests/eval_structure_generation/test_module_graph.py
+++ b/tests/eval_structure_generation/test_module_graph.py
@@ -44,7 +44,7 @@ def modules_and_ast(
 
 @pytest.fixture(scope="module")
 def imports(modules_and_ast: Tuple[List[str], List[NamedModule]]) -> Sequence[Import]:
-    return ImportConverter().convert(modules_and_ast[1])
+    return ImportConverter().convert(modules_and_ast[1], False, "", set())
 
 
 @pytest.fixture(scope="module")
diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py
index 1f73a69..f2fcb75 100644
--- a/tests/integration/test_integration.py
+++ b/tests/integration/test_integration.py
@@ -1,13 +1,17 @@
 from __future__ import annotations
 
+import os
+
 import pytest
 from integration.interesting_rules_for_tests import (
     partial_name_match_test_cases,
     single_rule_subject_multiple_rule_objects_error_message_test_cases,
     single_rule_subject_single_rule_object_error_message_test_cases,
 )
+from resources import root_module_mismatch_project
+from resources.root_module_mismatch_project import app
 
-from pytestarch import EvaluableArchitecture, Rule
+from pytestarch import EvaluableArchitecture, Rule, get_evaluable_architecture
 
 
 @pytest.mark.parametrize(
@@ -48,3 +52,51 @@ def test_rule_violation_correctly_detected_for_partial_name_matches(
     else:
         rule.assert_applies(graph_based_on_string_module_names)
         # no exception
+
+
+def test_root_module_mismatch_handled_as_expected() -> None:
+    evaluable = get_evaluable_architecture(
+        os.path.dirname(root_module_mismatch_project.__file__),
+        os.path.dirname(app.__file__),
+        ("*__pycache__", "*__init__.py", "*Test.py"),
+    )
+
+    rule = (
+        Rule()
+        .modules_that()
+        .are_sub_modules_of("root_module_mismatch_project.app.red")
+        .should_not()
+        .be_imported_by_modules_that()
+        .are_sub_modules_of("root_module_mismatch_project.app.green")
+    )
+
+    error_message = (
+        '"root_module_mismatch_project.app.red.red" is imported by "root_module_mismatch_project.app.green.green".\n'
+        '"root_module_mismatch_project.app.red.red2" is imported by "root_module_mismatch_project.app.green.green".\n'
+        '"root_module_mismatch_project.app.red.red3" is imported by "root_module_mismatch_project.app.green.green".'
+    )
+    with pytest.raises(AssertionError, match=error_message):
+        rule.assert_applies(evaluable)
+
+
+def test_root_module_match_handled_as_expected() -> None:
+    evaluable = get_evaluable_architecture(
+        os.path.dirname(app.__file__),
+        os.path.dirname(app.__file__),
+        ("*__pycache__", "*__init__.py", "*Test.py"),
+    )
+
+    rule = (
+        Rule()
+        .modules_that()
+        .are_named("app.red")
+        .should_not()
+        .be_imported_by_modules_that()
+        .are_sub_modules_of("app.green")
+    )
+
+    with pytest.raises(
+        AssertionError,
+        match='"app.red.red" is imported by "app.green.green".\n"app.red.red2" is imported by "app.green.green".\n"app.red.red3" is imported by "app.green.green".',
+    ):
+        rule.assert_applies(evaluable)
diff --git a/tests/resources/root_module_mismatch_project/__init__.py b/tests/resources/root_module_mismatch_project/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/resources/root_module_mismatch_project/app/__init__.py b/tests/resources/root_module_mismatch_project/app/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/resources/root_module_mismatch_project/app/green/__init__.py b/tests/resources/root_module_mismatch_project/app/green/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/resources/root_module_mismatch_project/app/green/green.py b/tests/resources/root_module_mismatch_project/app/green/green.py
new file mode 100644
index 0000000..5600b4e
--- /dev/null
+++ b/tests/resources/root_module_mismatch_project/app/green/green.py
@@ -0,0 +1,10 @@
+from __future__ import annotations
+
+import app.red.red3  # type: ignore  # noqa: F401
+from app.red.red import Red  # type: ignore
+
+from ..red.red2 import Red2  # type: ignore
+
+Red()
+Red2()
+Red3()  # type: ignore # noqa: F821
diff --git a/tests/resources/root_module_mismatch_project/app/red/__init__.py b/tests/resources/root_module_mismatch_project/app/red/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/resources/root_module_mismatch_project/app/red/red.py b/tests/resources/root_module_mismatch_project/app/red/red.py
new file mode 100644
index 0000000..51c13ac
--- /dev/null
+++ b/tests/resources/root_module_mismatch_project/app/red/red.py
@@ -0,0 +1,5 @@
+from __future__ import annotations
+
+
+class Red:
+    pass
diff --git a/tests/resources/root_module_mismatch_project/app/red/red2.py b/tests/resources/root_module_mismatch_project/app/red/red2.py
new file mode 100644
index 0000000..3205966
--- /dev/null
+++ b/tests/resources/root_module_mismatch_project/app/red/red2.py
@@ -0,0 +1,5 @@
+from __future__ import annotations
+
+
+class Red2:
+    pass
diff --git a/tests/resources/root_module_mismatch_project/app/red/red3.py b/tests/resources/root_module_mismatch_project/app/red/red3.py
new file mode 100644
index 0000000..49f13c6
--- /dev/null
+++ b/tests/resources/root_module_mismatch_project/app/red/red3.py
@@ -0,0 +1,5 @@
+from __future__ import annotations
+
+
+class Red3:
+    pass