-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Support PEP420 (implicit namespace packages) as --pyargs
target.
#13426
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Support PEP420 (implicit namespace packages) as `--pyargs` target when `consider_namespace_packages = true` is set in the config. | ||
Previously, this option only impacted package names, now it also impacts tests discovery. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -774,6 +774,9 @@ def perform_collect( | |
self._collection_cache = {} | ||
self.items = [] | ||
items: Sequence[nodes.Item | nodes.Collector] = self.items | ||
consider_namespace_packages: bool = self.config.getini( | ||
"consider_namespace_packages" | ||
) | ||
try: | ||
initialpaths: list[Path] = [] | ||
initialpaths_with_parents: list[Path] = [] | ||
|
@@ -782,6 +785,7 @@ def perform_collect( | |
self.config.invocation_params.dir, | ||
arg, | ||
as_pypath=self.config.option.pyargs, | ||
consider_namespace_packages=consider_namespace_packages, | ||
) | ||
self._initial_parts.append(collection_argument) | ||
initialpaths.append(collection_argument.path) | ||
|
@@ -981,7 +985,9 @@ def genitems(self, node: nodes.Item | nodes.Collector) -> Iterator[nodes.Item]: | |
node.ihook.pytest_collectreport(report=rep) | ||
|
||
|
||
def search_pypath(module_name: str) -> str | None: | ||
def search_pypath( | ||
module_name: str, *, consider_namespace_packages: bool = False | ||
) -> str | None: | ||
"""Search sys.path for the given a dotted module name, and return its file | ||
system path if found.""" | ||
try: | ||
|
@@ -991,13 +997,29 @@ def search_pypath(module_name: str) -> str | None: | |
# ValueError: not a module name | ||
except (AttributeError, ImportError, ValueError): | ||
return None | ||
if spec is None or spec.origin is None or spec.origin == "namespace": | ||
|
||
if spec is None: | ||
return None | ||
elif spec.submodule_search_locations: | ||
return os.path.dirname(spec.origin) | ||
else: | ||
|
||
if ( | ||
spec.submodule_search_locations is None | ||
or len(spec.submodule_search_locations) == 0 | ||
): | ||
# Must be a simple module. | ||
return spec.origin | ||
|
||
if consider_namespace_packages: | ||
# If submodule_search_locations is set, it's a package (regular or namespace). | ||
# Typically there is a single entry, but documentation claims it can be empty too | ||
# (e.g. if the package has no physical location). | ||
return spec.submodule_search_locations[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps instead of just returning it blindly, we should check if this is a directory before returning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I looked at it:
IMO it's fine to keep it as is, especially considering now this is hidden behind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree... the docs might say "a path" or "a directory", but given it is a string, who knows what some loader/package might be returning... |
||
|
||
if spec.origin is None: | ||
# This is only the case for namespace packages | ||
return None | ||
|
||
return os.path.dirname(spec.origin) | ||
|
||
|
||
@dataclasses.dataclass(frozen=True) | ||
class CollectionArgument: | ||
|
@@ -1009,7 +1031,11 @@ class CollectionArgument: | |
|
||
|
||
def resolve_collection_argument( | ||
invocation_path: Path, arg: str, *, as_pypath: bool = False | ||
invocation_path: Path, | ||
arg: str, | ||
*, | ||
as_pypath: bool = False, | ||
consider_namespace_packages: bool = False, | ||
) -> CollectionArgument: | ||
"""Parse path arguments optionally containing selection parts and return (fspath, names). | ||
|
||
|
@@ -1049,7 +1075,9 @@ def resolve_collection_argument( | |
parts[-1] = f"{parts[-1]}{squacket}{rest}" | ||
module_name = None | ||
if as_pypath: | ||
pyarg_strpath = search_pypath(strpath) | ||
pyarg_strpath = search_pypath( | ||
strpath, consider_namespace_packages=consider_namespace_packages | ||
) | ||
if pyarg_strpath is not None: | ||
module_name = strpath | ||
strpath = pyarg_strpath | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -169,8 +169,13 @@ def test_dir(self, invocation_path: Path) -> None: | |||
): | ||||
resolve_collection_argument(invocation_path, "src/pkg::foo::bar") | ||||
|
||||
def test_pypath(self, invocation_path: Path) -> None: | ||||
@pytest.mark.parametrize("namespace_package", [False, True]) | ||||
def test_pypath(self, namespace_package: bool, invocation_path: Path) -> None: | ||||
"""Dotted name and parts.""" | ||||
if namespace_package: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of piggybacked on the existing test that covers this functionality, but not sure whether it's a bit dirty! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @karlicoss, I think it is perfectly fine to reuse an existing test like this one, thanks! However functionality related to collection and packages tends to break complex test suites in subtle ways... been there done that. But to be honest not sure how we can even foresee how this will be handled in the wild, unless we put it in the wild. I'm considering adding this behind a feature flag, so if it causes havoc, we can optionally revert it... On the other hand, we can just bite the bullet and see. If this causes massive breakage, we can always revert the patch and make a hot fix. Just thinking aloud here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, agree, it makes a lot of sense! From experience, such random breakages are pretty annoying. Happy to implement a feature flag and add documentation for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been thinking about the flag name & documentation.. and actually, perhaps it makes sense to simply reuse From my experience with it, right now only helps with package/module names. E.g. if there is a test inside
And we run It correctly discovers tests in both cases, but.
This is consistent with
However, according to https://docs.pytest.org/en/stable/reference/reference.html#confval-consider_namespace_packages
So the docs say it "should attempt to idenfity when collecting", but I'm not sure what this actually means here. E.g. if you have the following hierarchy:
The only difference is
From a lurk in pytest code, to me it feels that indeed it has to do more with naming than test discovery -- e.g. I think stuff in this file is mostly after we already collected candidate files to run? Line 494 in 83536b4
So perhaps if I just reuse This also limits the potential impact from the change -- there are "only" 400ish matches for this setting on github https://github.com/search?q=consider_namespace_packages&type=code (tiny amount comparing to total pytest users), and I'd imagine people who use this setting know what they are doing anyway. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @nicoddemus --- any thoughts on this? :) ^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @karlicoss, sorry I missed your comment. Yeah I think reusing the option makes sense, and the impact is has a small radius as you mention. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Reused the option, feels like it's much more consistent now! I also added an extra 'end-to-end' test for |
||||
# Namespace package doesn't have to contain __init__py | ||||
(invocation_path / "src/pkg/__init__.py").unlink() | ||||
|
||||
assert resolve_collection_argument( | ||||
invocation_path, "pkg.test", as_pypath=True | ||||
) == CollectionArgument( | ||||
|
@@ -186,7 +191,10 @@ def test_pypath(self, invocation_path: Path) -> None: | |||
module_name="pkg.test", | ||||
) | ||||
assert resolve_collection_argument( | ||||
invocation_path, "pkg", as_pypath=True | ||||
invocation_path, | ||||
"pkg", | ||||
as_pypath=True, | ||||
consider_namespace_packages=namespace_package, | ||||
) == CollectionArgument( | ||||
path=invocation_path / "src/pkg", | ||||
parts=[], | ||||
|
@@ -197,7 +205,10 @@ def test_pypath(self, invocation_path: Path) -> None: | |||
UsageError, match=r"package argument cannot contain :: selection parts" | ||||
): | ||||
resolve_collection_argument( | ||||
invocation_path, "pkg::foo::bar", as_pypath=True | ||||
invocation_path, | ||||
"pkg::foo::bar", | ||||
as_pypath=True, | ||||
consider_namespace_packages=namespace_package, | ||||
) | ||||
|
||||
def test_parametrized_name_with_colons(self, invocation_path: Path) -> None: | ||||
|
Uh oh!
There was an error while loading. Please reload this page.