-
-
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
avoid using __file__ in pytest_plugin_registered as can be wrong on Windows #11825
avoid using __file__ in pytest_plugin_registered as can be wrong on Windows #11825
Conversation
0d169d8
to
ef99b0e
Compare
src/_pytest/fixtures.py
Outdated
nodeid = "" | ||
if os.sep != nodes.SEP: | ||
nodeid = nodeid.replace(os.sep, nodes.SEP) | ||
plugin_name = self.config.pluginmanager.get_name(plugin) |
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.
@bluetech Is that what you meant in #11816 (comment)?
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.
Yep, exactly, this LGTM.
Test failures seem unrelated. "hypothesis.errors.HypothesisSideeffectWarning: Slow code in plugin: avoid lazy evaluation of text() at import time!" never seen this before. Unless the get_name
slowness is actually the culprit... I will re-run the jobs later.
As for optimizing the get_name
call, I can do it in a separate PR, unless you want to do it here already. The best approach I think is to add a plugin_name
parameter to the pytest_plugin_registered
hook.
No idea what this means so if you could handle it in another MR, that would be great. |
The Hypothesis issue seems to reproduce when re-running the jobs, though some jobs pass. The (existing, seems to be unrelated to this PR) test is: @hypothesis.given(strategies.text() | strategies.binary())
@hypothesis.settings(
deadline=400.0
) # very close to std deadline and CI boxes are not reliable in CPU power
def test_idval_hypothesis(self, value) -> None:
escaped = IdMaker([], [], None, None, None, None, None)._idval(value, "a", 6)
assert isinstance(escaped, str)
escaped.encode("ascii") The error is:
@Zac-HD Maybe you have an idea? The test seems run of the mill to me and I don't see any import-time evaluation going on but I don't have much experience with Hypothesis. |
We just added that warning last night, and I suspect a pytest-specific interaction... I suggest globally ignoring |
Otherwise mypy doesn't fully recognize pluggy's typing for some reason or another.
ef99b0e
to
d0c531d
Compare
Added the change to the hook. Did it in this PR to make backporting easier. |
…ed` hook We have a use case for this in the next commit. The name can be obtained by using `manager.get_name(plugin)`, however this is currently O(num plugins) in pluggy, which would be good to avoid. Besides, it seems generally useful.
…gin_name` hook parameter
d0c531d
to
9ea2e0a
Compare
closes #11816
Alternative fix to #11821.
The plugin name is equal to the path for conftest plugins (suggested in #11816 (comment)). Use that instead of
plugin.__file__
.Test with the h5py CI that was broken by the issue: h5py/h5py#2370
Another way of testing this MR: