Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: add testcase that shows that pyreverse will not extract the inheritance link for a flat folder as described in #7686 #9693

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Hannoma
Copy link

@Hannoma Hannoma commented Jun 4, 2024

Type of Changes

Type
🐛 Bug fix

Description

As described in #7686, pyreverse does not extract the inheritance link for classes living in different files. In this PR I haved added the possibility to easily write testcases for pyreverse to generate a diagram for a complete directory / module. As far as I can tell this was not possible before just using the functional tests.

I also have a possible fix in mind for this issue, but am not sure, if I should add it in this PR or open another one. As @DudeNr33 suggested, I started with writing the test to reproduce the described issue.

Refs #7686

@Hannoma Hannoma requested a review from DudeNr33 as a code owner June 4, 2024 13:40
@Hannoma Hannoma changed the title tests: add testcase that shows that pyreverse will not extract the inheritance link for a flat folder as described in https://github.com/pylint-dev/pylint/issues/7686 tests: add testcase that shows that pyreverse will not extract the inheritance link for a flat folder as described in #7686 Jun 4, 2024
@Hannoma
Copy link
Author

Hannoma commented Jun 4, 2024

What I have found out while debugging:
When the node.ancestors(recurs=False) function is called in diagrams.py lines 213-219

            # inheritance link
            for par_node in node.ancestors(recurs=False):
                try:
                    par_obj = self.object_from_node(par_node)
                    self.add_relationship(obj, par_obj, "specialization")
                except KeyError:
                    continue

the ancestors are not extracted correctly as the cached InferenceContext maps the statement of the name of the base classes to "Uninferable"
image

@Hannoma
Copy link
Author

Hannoma commented Jun 4, 2024

As I really do not know what is going on there in astroid, my quick and dirty solution would be to extract the correct base classes via their name:

            # inheritance links
            for par_node in node.bases:
                # Name nodes are used for simple types, e.g. class A(B):
                if isinstance(par_node, astroid.Name):
                    try:
                        par_obj = self.classe(par_node.name)
                        self.add_relationship(obj, par_obj, "specialization")
                    except KeyError:
                        continue
                # Subscript is used for generic types, e.g. class A(Generic[T]):
                if isinstance(par_node, astroid.Subscript):
                    try:
                        par_obj = self.classe(par_node.value.name)
                        self.add_relationship(obj, par_obj, "specialization")
                    except KeyError:
                        continue

I have pushed this simple fix to https://github.com/ANTICIPATE-GmbH/pylint/tree/fix/7686/extract-inheritance-relations-correctly. Note that with this fix, all test cases complete without issues.

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added pyreverse Related to pyreverse component Needs decision 🔒 Needs a decision before implemention or rejection Needs design proposal 🔒 This is a huge feature, some discussion should happen before a PR is proposed and removed Needs design proposal 🔒 This is a huge feature, some discussion should happen before a PR is proposed labels Jun 4, 2024
Copy link
Collaborator

@DudeNr33 DudeNr33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this up. I have not found the time yet to check if the problem itself could/should be fixed in astroid rather than pylint, so I will just focus on the testing extension for now:

I would indeed suggest to keep the testing extension and the concrete fix as two separate merge requests. I left some first thoughts.

It would be great if this could be used for functional tests of package diagrams as well, though this could be added later in a separate MR.

pylint/testutils/pyreverse.py Outdated Show resolved Hide resolved
pylint/testutils/pyreverse.py Outdated Show resolved Hide resolved
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit d99f984

@DudeNr33
Copy link
Collaborator

I tried it out locally and I quite like it.
As mentioned I would like to expand on this to also include a functional test harness for package diagrams.
There already exists some test data under tests/pyreverse/functional/package_diagrams, which however does not build upon test_pyreverse_functional.py to collect the test data.

With your change and some very minor modifications, we can move these files into the new tests/pyreverse/functional/packages directory (we can do that in a follow up PR, just want to note down what I did locally):

diff --git a/pylint/testutils/pyreverse.py b/pylint/testutils/pyreverse.py
index 9e6647183..03840944e 100644
--- a/pylint/testutils/pyreverse.py
+++ b/pylint/testutils/pyreverse.py
@@ -72,6 +72,7 @@ class TestFileOptions(TypedDict):
     source_roots: list[str]
     output_formats: list[str]
     command_line_args: list[str]
+    diagram_type: str
 
 
 class FunctionalPyreverseTestfile(NamedTuple):
@@ -150,4 +151,5 @@ def _read_config(config_file: Path) -> TestFileOptions:
         "command_line_args": shlex.split(
             config.get("testoptions", "command_line_args", fallback="")
         ),
+        "diagram_type": config.get("testoptions", "diagram_type", fallback="classes"),
     }
diff --git a/tests/pyreverse/test_pyreverse_functional.py b/tests/pyreverse/test_pyreverse_functional.py
index beeb0330b..e02574463 100644
--- a/tests/pyreverse/test_pyreverse_functional.py
+++ b/tests/pyreverse/test_pyreverse_functional.py
@@ -86,5 +86,5 @@ def test_packages(test_package: FunctionalPyreverseTestfile, tmp_path: Path) ->
             Run(args)
         assert sys_exit.value.code == 0
         assert output_file.read_text(encoding="utf8") == (
-            tmp_path / f"classes.{output_format}"
+            tmp_path / f"{test_package.options['diagram_type']}.{output_format}"
         ).read_text(encoding="utf8")

This essentially just adds an option to the .rc file so that the author can specify if his tests generate a class or a package diagram.

As a next step regarding this MR, I think we can simplify the code a bit. For example, the get_functional_test_files() and get_functional_test_packages() in the testutils package share some common logic to handle the .rc files, and the test_class_diagrams() and test_packages() in test_pyreverse_functional as well.

For the suggested fix, I am hesitant to judge if this is the best way to fix it, as I currently don't find the time to work myself back into astroid and debug this issue to understand what is going on. Maybe one of the other maintainers has time to take a look (maybe @nickdrozd?) , otherwise I have to ask for your patience until I can take the time to properly look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs decision 🔒 Needs a decision before implemention or rejection pyreverse Related to pyreverse component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants