-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Do not include the root session with -k
#7046
Do not include the root session with -k
#7046
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The issue definitely seems like something that needs fixing, I'm not confident enough to evaluate whether this is the best solution though - hopefully others will chime in)
src/_pytest/mark/legacy.py
Outdated
@@ -46,7 +46,7 @@ def from_item(cls, item: "Item") -> "KeywordMapping": | |||
import pytest | |||
|
|||
for item in item.listchain(): | |||
if not isinstance(item, pytest.Instance): | |||
if item.parent and not isinstance(item, pytest.Instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is item.parent
the canonical way to check for the root node? And are we sure we always want to exclude the root node from keyword matching?
In any case a comment explaining the item.parent
condition would be helpful I think since it's not immediately obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous fix is better, because it explicitly skips Session
in the code. 👍
81dc5d5
to
f5228c4
Compare
testing/test_mark.py
Outdated
assert_test_is_not_selected("+") | ||
assert_test_is_not_selected("..") | ||
# should not match against session | ||
assert_test_is_not_selected(testdir.tmpdir.basename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to remove parametrization because of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold up, the fix is not correct yet.
This one should go in, and it classifies as breaking |
Previously it contained the entire path, which made '-k' match against any name in the full path of the package. Fix pytest-dev#7040
f5228c4
to
c5b367b
Compare
@@ -568,8 +568,7 @@ def __init__( | |||
nodes.FSCollector.__init__( | |||
self, fspath, parent=parent, config=config, session=session, nodeid=nodeid | |||
) | |||
|
|||
self.name = fspath.dirname | |||
self.name = os.path.basename(str(fspath.dirname)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that one looks like a breaking change in isolation, does it bring us in line with other details or is it a new difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crosschecked, it brings us in line
Fixes #7040.