python: change `Package` to no longer be a `Module`/`File` #11138

bluetech · 2023-06-24T16:49:18Z

I'll leave detailed notes on the changes below to hopefully make them easier to understand.

bluetech · 2023-06-24T16:50:54Z

src/_pytest/cacheprovider.py

@@ -228,12 +228,7 @@ def pytest_make_collect_report(self, collector: nodes.Collector):

            # Use stable sort to priorize last failed.
            def sort_key(node: Union[nodes.Item, nodes.Collector]) -> bool:
-                # Package.path is the __init__.py file, we need the directory.


Simplification - previous special case for Package no longer needed

bluetech · 2023-06-24T16:51:05Z

src/_pytest/cacheprovider.py

@@ -280,9 +275,7 @@ def __init__(self, lfplugin: "LFPlugin") -> None:
    def pytest_make_collect_report(
        self, collector: nodes.Collector
    ) -> Optional[CollectReport]:
-        # Packages are Files, but we only want to skip test-bearing Files,


Simplification - previous special case for Package no longer needed

bluetech · 2023-06-24T16:53:29Z

src/_pytest/fixtures.py

@@ -120,9 +120,8 @@ def get_scope_package(
    from _pytest.python import Package

    current: Optional[Union[nodes.Item, nodes.Collector]] = node
-    fixture_package_name = "{}/{}".format(fixturedef.baseid, "__init__.py")


No longer need special code to adjust to the fact the package's path is the __init__.py.

This change will be relevant for fixing #10993 and other similar bugs.

bluetech · 2023-06-24T16:55:20Z

src/_pytest/main.py

@@ -571,6 +572,17 @@ def _recurse(self, direntry: "os.DirEntry[str]") -> bool:
            return False
        return True

+    def _collectpackage(self, fspath: Path) -> Optional["Package"]:


Package is now no longer collected as a Module, so need to do it manually.

This is temporary, will be entirely revamped using the planned pytest_collect_directory hook (see #7777).

bluetech · 2023-06-24T16:56:16Z

src/_pytest/main.py


        for argpath, names in self._initial_parts:
            self.trace("processing argument", (argpath, names))
            self.trace.root.indent += 1

            # Start with a Session root, and delve to argpath item (dir or file)
            # and stack all Packages found on the way.
-            # No point in finding packages when collecting doctests.
-            if not self.config.getoption("doctestmodules", False):


I haven't been able to understand this optimization, and it hinders the implementation, so I removed it. It will become irrelevant in upcoming changes anyway.

bluetech · 2023-06-24T17:04:50Z

src/_pytest/python.py

@@ -521,11 +518,62 @@ def _genfunctions(self, name: str, funcobj) -> Iterator["Function"]:
                )


+def importtestmodule(


Refactor: moving this code from a method to a standalone function because we need to reuse it. No logic changes.

bluetech · 2023-06-24T17:05:51Z

src/_pytest/python.py

@@ -681,22 +682,24 @@ def __init__(
            session=session,
            nodeid=nodeid,
        )
-        self.name = self.path.parent.name
+        self.name = self.path.name


The path is now the directory, not the __init__.py. The name is the name of the directory, so no need for the parent anymore.

bluetech · 2023-06-24T17:07:40Z

src/_pytest/python.py


    def setup(self) -> None:
+        init_mod = importtestmodule(self.path / "__init__.py", self.config)


This was previously handled by Module importing its module file (the __init__.py file), now we need to do it ad-hoc.

As discussed in the issue, I think we should consider deprecating setup_module/teardown_module in __init__.py files for Packages, it's pretty obscure, but for now I retained full compat.

makes me wonder if we want to consider deprecating xunit style setup methods alltogether as they are practically better served by fixtures

I think it will be too disruptive at this point, but maybe with nose gone we can start considering this as well. Overall though I think it's not worth deprecating, I know a lot of people use them.

This particular case though (setup_module xunit fixtures in the __init__.py file being package scoped) is more dubious than the others, I'd be surprised to learn that anyone knows about it at all. So we might want to consider deprecating it specifically.

bluetech · 2023-06-24T17:08:50Z

src/_pytest/python.py

        # Always collect the __init__ first.
-        if self.session.isinitpath(self.path) or path_matches_patterns(


Since __init__.py is no longer treated special by pytest_pycollect_makemodule, can just use _collectfile for it.

bluetech · 2023-06-24T17:09:17Z

src/_pytest/python.py

@@ -733,21 +736,16 @@ def _collectfile(
        return ihook.pytest_collect_file(file_path=fspath, parent=self)  # type: ignore[no-any-return]

    def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]:
-        this_path = self.path.parent


self.path is now this path (the directory)

bluetech · 2023-07-28T07:00:06Z

Rebased to fix conflicts
Added the change in get_parametrized_fixture_keys, which I had missed before

nicoddemus

Looks great, thanks @bluetech!

bluetech · 2023-07-28T11:33:07Z

Thanks a lot for the review @nicoddemus!

I'll try to get the coverage passing (in separate PRs) then merge.

RonnyPfannschmidt

i love how much this cleans things up, awesome 👍

Fix pytest-dev#11137.

Pierre-Sassoulas · 2024-05-01T12:35:35Z

This commit prevents upgrading pytest above 8.0.0 in pylint (pylint-dev/pylint#9576) . Am I correct in assuming that the order the tests are discovered and launched changed (with the change to sort_key) ? I assume it could have revealed an existing test contamination issue in pylint's test suite. I see a visit call and occurrence of nodes.Collector but pytest does not modify the ast during test collection, right ?

RonnyPfannschmidt · 2024-05-01T12:54:13Z

AST modification may occur during collection in case assertion rewriting needs to act

bluetech · 2024-05-01T12:55:33Z

@Pierre-Sassoulas from a quick look it looks like maybe something changed with regards to the AudioTime precedence test: https://github.com/search?q=repo%3Apylint-dev%2Fpylint%20AudioTime&type=code

Are you able to run this test https://github.com/pylint-dev/pylint/blob/82ef6475e665ca8ae1542822a485c98a067e1834/tests/test_regr.py#L59 "manually" (outside of pytest) and see if it passes?

Suggestion from pytest-dev/pytest#11138 (comment) With 'pytest': FAILED tests/test_precedence.py::test_package - AssertionError: E: 21: Module 'package.AudioTime' has no 'DECIMAL' member<function Equals.<locals>.<lambda> at 0x76c566741750> With 'python tests/test_precedence.py': Checked ['package.__init__'] successfully Checked ['precedence_test'] successfully Checked ['import_package_subpackage_module'] successfully Checked ['pylint.checkers.__init__'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/classdoc_usage.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/module_global.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/decimal_inference.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/absimp/string.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/bad_package'] successfully

Suggestion from pytest-dev/pytest#11138 (comment) With 'pytest' (launching the whole pylint test suite): FAILED tests/test_precedence.py::test_package - AssertionError: E: 21: Module 'package.AudioTime' has no 'DECIMAL' member<function Equals.<locals>.<lambda> at 0x76c566741750> With 'pytest tests/test_precedence.py': tests/test_precedence.py . [100%] ============================================================================================ 1 passed in 1.04s ============================================================================================= With 'python tests/test_precedence.py': Checked ['package.__init__'] successfully Checked ['precedence_test'] successfully Checked ['import_package_subpackage_module'] successfully Checked ['pylint.checkers.__init__'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/classdoc_usage.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/module_global.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/decimal_inference.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/absimp/string.py'] successfully Checked ['/home/pierre/pylint/tests/regrtest_data/bad_package'] successfully

Pierre-Sassoulas · 2024-05-01T15:58:52Z

Thank you for the quick answers, very helpful !

AST modification may occur during collection in case assertion rewriting needs to act

Interesting, I wonder if we have some pylint features that do not work without a pytest test collection prior to the analysis 😄

Are you able to run this test https://github.com/pylint-dev/pylint/blob/82ef6475e665ca8ae1542822a485c98a067e1834/tests/test_regr.py#L59 "manually" (outside of pytest) and see if it passes?

Tried that in pylint-dev/pylint@49a7a61, it seems this is indeed not passing due to pytest collection (but only when it collects the whole pylint test suite).

bluetech added this to the 8.0 milestone Jun 24, 2023

bluetech commented Jun 24, 2023

View reviewed changes

bluetech force-pushed the pkg-mod branch from 72457dc to bfca39d Compare July 28, 2023 06:59

bluetech mentioned this pull request Jul 28, 2023

fixtures: fix crash when parametrize(scope="package") is used without a Package #11256

Merged

nicoddemus approved these changes Jul 28, 2023

View reviewed changes

RonnyPfannschmidt approved these changes Jul 28, 2023

View reviewed changes

python: change Package to no longer be a Module/File

a21fb87

Fix pytest-dev#11137.

bluetech force-pushed the pkg-mod branch from bfca39d to a21fb87 Compare July 28, 2023 19:49

bluetech merged commit 485c555 into pytest-dev:main Jul 28, 2023
24 of 25 checks passed

bluetech deleted the pkg-mod branch July 28, 2023 20:15

Pierre-Sassoulas mentioned this pull request May 1, 2024

Update pytest requirement from ~=7.4 to ~=8.2 pylint-dev/pylint#9576

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python: change `Package` to no longer be a `Module`/`File` #11138

python: change `Package` to no longer be a `Module`/`File` #11138

bluetech commented Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

RonnyPfannschmidt Jul 28, 2023

bluetech Jul 28, 2023

bluetech Jun 24, 2023

bluetech Jun 24, 2023

bluetech commented Jul 28, 2023

nicoddemus left a comment

bluetech commented Jul 28, 2023

RonnyPfannschmidt left a comment

Pierre-Sassoulas commented May 1, 2024

RonnyPfannschmidt commented May 1, 2024

bluetech commented May 1, 2024

Pierre-Sassoulas commented May 1, 2024

		@@ -521,11 +518,62 @@ def _genfunctions(self, name: str, funcobj) -> Iterator["Function"]:
		)


		def importtestmodule(


		def setup(self) -> None:
		init_mod = importtestmodule(self.path / "__init__.py", self.config)

		# Always collect the __init__ first.
		if self.session.isinitpath(self.path) or path_matches_patterns(

python: change Package to no longer be a Module/File #11138

python: change Package to no longer be a Module/File #11138

Conversation

bluetech commented Jun 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech commented Jul 28, 2023

nicoddemus left a comment

Choose a reason for hiding this comment

bluetech commented Jul 28, 2023

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented May 1, 2024

RonnyPfannschmidt commented May 1, 2024

bluetech commented May 1, 2024

Pierre-Sassoulas commented May 1, 2024

python: change `Package` to no longer be a `Module`/`File` #11138

python: change `Package` to no longer be a `Module`/`File` #11138