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

python: change pytest pkg/__init__.py to only collect the __init__.py Module #11042

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

bluetech
Copy link
Member

Previously it would collect the entire package, but this is not what users (including me) expect.

Refs #3749
Fixes #8976
Fixes #9263
Fixes #9313


Technically I'd say this is a bug fix, but I'm not entirely sure the previous behavior wasn't intentional (there are certainly several tests to the contrary). And it also has potential of breaking users who somehow rely on the previous behavior. Therefore I've marked this as breaking, and it should not be merged before we decide the next version will be a major. Still, acks would be good already :)

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

# Always collect the __init__ first.
if path_matches_patterns(self.path, self.config.getini("python_files")):
if (
self.session.isinitpath(self.path)
Copy link
Member Author

Choose a reason for hiding this comment

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

self.path here is the __init__.py file (== the path of the Package).

This says, if pkg/__init__.py was specified directly (e.g. pytest pkg/__init__.py), then collect its Module even if __init__.py is not a recognized pattern in python_files config.

The relevance of this to the bug is the following code in Session:

pytest/src/_pytest/main.py

Lines 805 to 818 in 4f3f36c

# If __init__.py was the only file requested, then the matched
# node will be the corresponding Package (by default), and the
# first yielded item will be the __init__ Module itself, so
# just use that. If this special case isn't taken, then all the
# files in the package will be yielded.
if argpath.name == "__init__.py" and isinstance(matching[0], Package):
try:
yield next(iter(matching[0].collect()))
except StopIteration:
# The package collects nothing with only an __init__.py
# file in it, which gets ignored by the default
# "python_files" option.
pass
continue

That code is currently broken.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM!

Should we add some note to the docs themselves, besides the CHANGELOG?

@bluetech
Copy link
Member Author

Should we add some note to the docs themselves, besides the CHANGELOG?

Any suggestion where? I was thinking of adding a note to the deprecations page, but that only lists "Deprecated features" and "Removed features". Maybe change "Removed features" to "Removed features and breaking changes" and add it there?

@nicoddemus
Copy link
Member

Any suggestion where? I was thinking of adding a note to the deprecations page, but that only lists "Deprecated features" and "Removed features". Maybe change "Removed features" to "Removed features and breaking changes" and add it there?

Yeah I was quickly browsing the docs and could not find any in-depth discussion about collection...

I think your suggestion is good. 👍

….py` Module

Previously it would collect the entire package, but this is not what
users expect.

Refs pytest-dev#3749
Fixes pytest-dev#8976
Fixes pytest-dev#9263
Fixes pytest-dev#9313
@bluetech bluetech merged commit fe51121 into pytest-dev:main Jun 23, 2023
27 checks passed
@bluetech bluetech deleted the pkg-init-argpath branch June 23, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants