-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Disable assertion rewriting external modules #13421
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
base: main
Are you sure you want to change the base?
Disable assertion rewriting external modules #13421
Conversation
Need to squash commits - OK to close, I'll reopen a new one with one commit |
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.
thank you for getting this started
i believe we need to put the path handling into assertion state so it can correctly pass from the configuration and include the invocation dir/rootdir in a more safe manner than the current heusterics
23dcf0c
to
54f46d7
Compare
I skimmed through the issue (I'm short on time so I cannot do a more through research), but looking at the code is not immediately clear to me so thought I would ask: Note that we want to rewrite asserts for files belonging to a pytest plugin, even if they are not |
6a9b406
to
1592f85
Compare
At present time the fix is applied only for path which applies |
Added some tests for plugin rewriting, it works now |
16f3fc9
to
ba4263d
Compare
testing/test_assertrewrite.py
Outdated
with mock.patch.object(hook, "fnpats", ["*.py"]): | ||
assert hook.find_spec("file") is None | ||
|
||
def test_assert_rewrite_correct_for_conftfest( |
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.
mamy name it in another way - test_assert_rewrite_for_conftfest
4e1f8b9
to
db7dcd4
Compare
src/_pytest/assertion/__init__.py
Outdated
@@ -108,6 +109,19 @@ def __init__(self, config: Config, mode) -> None: | |||
self.trace = config.trace.root.get("assertion") | |||
self.hook: rewrite.AssertionRewritingHook | None = None | |||
|
|||
@property |
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.
this must mot use getwd instead use the invoction params
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.
then we cannot change it on runtime as far as invocation param for pytester changes rootpath
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.
sorry for the type - this code shoud use the rootdir or the invocationdir from the invocation params of the config
see https://docs.pytest.org/en/stable/reference/reference.html#pytest.Config.invocation_params as well as the config rootdir
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.
My error, I mean as far as Invocation Param is frozen - it can't be changed on runtime. Pytester starts after the config has been loaded. So to change the rootpath for pytester I need to rewrite the rootpath in someway. I could mock all invocation params or I could use getcwd alongside with config rootdir for the testing purpose.
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 complete object can be replaced with a changed 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.
ok. would be done)
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.
But is config.invocation_params.dir
the correct thing to use? If I execute the tests using pytest src/tests/foo.py
, I expect pytest to continue to rewrite the same files as if I execute just pytest
.
I feel we should use config.rootpath
here? What do you think @RonnyPfannschmidt ?
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.
indeed - the rootpath - else we miss imports to rewrite
3b36041
to
23e0d70
Compare
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.
Thanks @Tusenka,
Left some comments, please take a look.
src/_pytest/assertion/__init__.py
Outdated
self.hook: rewrite.AssertionRewritingHook | None = None | ||
|
||
@property | ||
def rootpath(self): |
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.
We already have rootpath
in Config
, but this property returns something different so this is a bit misleading. I suggest we rename it to something else to avoid this confusion, how about:
def rootpath(self): | |
def invocation_path(self): |
src/_pytest/assertion/__init__.py
Outdated
@@ -108,6 +109,19 @@ def __init__(self, config: Config, mode) -> None: | |||
self.trace = config.trace.root.get("assertion") | |||
self.hook: rewrite.AssertionRewritingHook | None = None | |||
|
|||
@property |
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.
But is config.invocation_params.dir
the correct thing to use? If I execute the tests using pytest src/tests/foo.py
, I expect pytest to continue to rewrite the same files as if I execute just pytest
.
I feel we should use config.rootpath
here? What do you think @RonnyPfannschmidt ?
@pytest.mark.skipif( | ||
sys.platform.startswith("win32"), reason="cannot remove cwd on Windows" | ||
) | ||
@pytest.mark.skipif( | ||
sys.platform.startswith("sunos5"), reason="cannot remove cwd on Solaris" | ||
) |
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.
Are these skipif
marks still necessary?
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 sunos still a supported python platform
As far as I'm aware sun is down
self, pytester: Pytester, hook: AssertionRewritingHook, monkeypatch | ||
) -> None: | ||
"""If test files contained outside the rootpath, then skip them""" | ||
pytester.makepyfile( |
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 would prefer to avoid mocking here, instead using a real scenario to ensure Python files outside of the rootpath do not have their assertions rewritten.
Please create a simple but real-world project using pytester
, something like:
myproject/
/venv
/lib
/external_lib.py
pyvenv.cfg
/src
main.py
/tests
conftest.py
test_main.py
pytest.ini
outside.py
pytest.ini
should have these contents:
[pytest]
python_files = *.py
Then execute pytest.runpytest()
, which will execute from the root of the above tree.
If I understand the objective of the issue correctly:
myproject/venv/lib/external_lib.py
: should not have assertions rewritten -- the file is inside a virtual environment and is not part of a pytest plugin. Thepyvenv.cfg
file is how pytest detects virtual environments.myproject/src/main.py
: rewritten -- insiderootpath
and matchespython_files
.myproject/tests/conftest.py
: rewritten --conftest.py
files are always rewritten.myproject/tests/test_main.py
: rewritten -- test files are always rewritten.outside.py
: should not have assertions rewritten -- outside therootpath
.
I think the scenario above should cover what needs to be tested.
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.
We need to include an external plugin here too, to test the same scenario as in test_assert_rewrite_correct_for_plugins
.
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've tried to create the structure along with a some conftest plugin. Even witout changes it doen't rewrite conftest plugins assertions
Example:
https://github.com/Tusenka/pytest/tree/disable_assertion_rewriting_external_modules_draft/testing/example_scripts/rewrite
However it sees only top-level plugins:
['anyio', '_hypothesis_ftz_detector', '_hypothesis_globals', '_hypothesis_pytestplugin', 'hypothesis', 'pytest_twisted', 'typeguard']
Logs for rewriting
Does it an expected behavior? How does pytest provide plugins which are needed to be rewritten at the import stage? May be I do something wrong
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.
It should rewrite the conftest files
If it doesn't then we have a bug we missed that needs a additional regression test
for more information, see https://pre-commit.ci
…ath to AssertState
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Bruno Oliveira <[email protected]>
for more information, see https://pre-commit.ci
26b5195
to
c4e1bae
Compare
Do we need to rewrite contest plugins? It's the not original behavior |
Conftest files that are part of the collection should rewrite as far as I remember |
src/_pytest/assertion/__init__.py
Outdated
@@ -110,8 +110,14 @@ class AssertionState: | |||
def __init__(self, config: Config, mode) -> None: | |||
self.mode = mode | |||
self.trace = config.trace.root.get("assertion") | |||
self.config = config |
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.
Let's perhaps just pass in the invocation details
Id like to avoid full config in more objects
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.
Hmm. I need to think.
In this case we couldn't change the invocation path for pytester at the simplest way.
The config file is loaded on the initial import stage. Pytester is loaded after that stage and still doesn't know anything about AssertionState.
I need to think what is the best way to do that.
src/_pytest/assertion/__init__.py
Outdated
@@ -108,6 +109,19 @@ def __init__(self, config: Config, mode) -> None: | |||
self.trace = config.trace.root.get("assertion") | |||
self.hook: rewrite.AssertionRewritingHook | None = None | |||
|
|||
@property |
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.
indeed - the rootpath - else we miss imports to rewrite
@@ -749,6 +750,13 @@ def chdir(self) -> None: | |||
This is done automatically upon instantiation. | |||
""" | |||
self._monkeypatch.chdir(self.path) | |||
self._monkeypatch.setattr( |
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.
do i gather correct that this bit is to alter the assertion rewtiter state
we should alter only that state, not the full pytest config of the surrounding pytest - this looks like a assertion rewriter shortcoming that needs a followup after this
but we certainly need to alter the assertion hook in sys metapath instead of a pytest config
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.
Pytester doesn't know anything about the AssertionState yet. In order to solve that I need to think in someway how to do this better
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.
We should go with a temporary solution and add a followup issue
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 this a artifact of a missing gitignore?
my understanding is that rewrite of conftest plugins was always happening as part of the original assertion hooking |
ace23c1
to
1d5a612
Compare
for more information, see https://pre-commit.ci
179b723
to
edcf484
Compare
config = pytester.parseconfig() | ||
state = AssertionState(config, "rewrite") | ||
assert state.invocation_path == str(config.invocation_params.dir) | ||
new_rootpath = str(pytester.path / "test") |
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.
Please stay with the pathlib api instead of switching to Str and then back
self, pytester: Pytester, hook: AssertionRewritingHook, monkeypatch | ||
) -> None: | ||
"""If test files contained outside the rootpath, then skip them""" | ||
pytester.makepyfile( |
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.
It should rewrite the conftest files
If it doesn't then we have a bug we missed that needs a additional regression test
@@ -749,6 +750,13 @@ def chdir(self) -> None: | |||
This is done automatically upon instantiation. | |||
""" | |||
self._monkeypatch.chdir(self.path) | |||
self._monkeypatch.setattr( |
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.
We should go with a temporary solution and add a followup issue
Disable assertion rewriting of external modules. Closes 13403.