-
-
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?
Changes from all commits
fb29583
14b0382
10a2709
fc0c87c
3918365
62fe69d
8671103
3beb48e
0015d0a
d5eb2a6
392a01d
ccb0dca
e0bbaa3
dbc5a59
23e0d70
559a5c0
a3cacb8
3358751
6ec5c30
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 @@ | ||
Disable assertion rewriting of external modules | ||
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -106,8 +106,14 @@ class AssertionState: | |||||
def __init__(self, config: Config, mode) -> None: | ||||||
self.mode = mode | ||||||
self.trace = config.trace.root.get("assertion") | ||||||
self.config = config | ||||||
self.hook: rewrite.AssertionRewritingHook | None = None | ||||||
|
||||||
@property | ||||||
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. this must mot use getwd instead use the invoction params 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. 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 commentThe 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 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. 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. But is I feel we should use |
||||||
def rootpath(self): | ||||||
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. We already have
Suggested change
|
||||||
"""Get current root path (current working dir)""" | ||||||
return str(self.config.invocation_params.dir) | ||||||
|
||||||
|
||||||
def install_importhook(config: Config) -> rewrite.AssertionRewritingHook: | ||||||
"""Try to install the rewrite hook, raise SystemError if it fails.""" | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -749,6 +749,15 @@ def chdir(self) -> None: | |||||||||||||
This is done automatically upon instantiation. | ||||||||||||||
""" | ||||||||||||||
self._monkeypatch.chdir(self.path) | ||||||||||||||
self._monkeypatch.setattr( | ||||||||||||||
self._request.config, | ||||||||||||||
"invocation_params", | ||||||||||||||
Config.InvocationParams( | ||||||||||||||
args=self._request.config.invocation_params.args, | ||||||||||||||
plugins=self._request.config.invocation_params.plugins, | ||||||||||||||
dir=Path(self._path), | ||||||||||||||
), | ||||||||||||||
Comment on lines
+755
to
+759
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.
Suggested change
|
||||||||||||||
) | ||||||||||||||
|
||||||||||||||
def _makefile( | ||||||||||||||
self, | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
import inspect | ||
import marshal | ||
import os | ||
from os import mkdir | ||
from pathlib import Path | ||
import py_compile | ||
import re | ||
|
@@ -35,6 +36,7 @@ | |
from _pytest.assertion.rewrite import rewrite_asserts | ||
from _pytest.config import Config | ||
from _pytest.config import ExitCode | ||
from _pytest.monkeypatch import MonkeyPatch | ||
from _pytest.pathlib import make_numbered_dir | ||
from _pytest.pytester import Pytester | ||
import pytest | ||
|
@@ -370,6 +372,7 @@ def test_rewrites_plugin_as_a_package(self, pytester: Pytester) -> None: | |
pytester.makeconftest('pytest_plugins = ["plugin"]') | ||
pytester.makepyfile("def test(special_asserter): special_asserter(1, 2)\n") | ||
result = pytester.runpytest() | ||
|
||
result.stdout.fnmatch_lines(["*assert 1 == 2*"]) | ||
|
||
def test_honors_pep_235(self, pytester: Pytester, monkeypatch) -> None: | ||
|
@@ -1294,6 +1297,34 @@ def test_meta_path(): | |
) | ||
assert pytester.runpytest().ret == 0 | ||
|
||
def test_rootpath_base(self, pytester: Pytester, monkeypatch: MonkeyPatch) -> None: | ||
"""Base cases for get rootpath from AssertionState""" | ||
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. The test name and docstring are a bit confusing to me, why "base cases"? Isn't this test just testing the |
||
from _pytest.assertion import AssertionState | ||
|
||
config = pytester.parseconfig() | ||
state = AssertionState(config, "rewrite") | ||
assert state.rootpath == str(config.invocation_params.dir) | ||
new_rootpath = str(pytester.path / "test") | ||
if not os.path.exists(new_rootpath): | ||
os.mkdir(new_rootpath) | ||
monkeypatch.setattr( | ||
config, | ||
"invocation_params", | ||
Config.InvocationParams( | ||
args=(), | ||
plugins=(), | ||
dir=Path(new_rootpath), | ||
), | ||
) | ||
state = AssertionState(config, "rewrite") | ||
assert state.rootpath == new_rootpath | ||
|
||
@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" | ||
) | ||
Comment on lines
+1322
to
+1327
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. Are these |
||
def test_write_pyc(self, pytester: Pytester, tmp_path) -> None: | ||
from _pytest.assertion import AssertionState | ||
from _pytest.assertion.rewrite import _write_pyc | ||
|
@@ -1971,6 +2002,95 @@ def test_simple_failure(): | |
assert hook.find_spec("file") is not None | ||
assert self.find_spec_calls == ["file"] | ||
|
||
def test_assert_rewrites_only_rootpath( | ||
self, pytester: Pytester, hook: AssertionRewritingHook, monkeypatch | ||
) -> None: | ||
"""Do not rewrite assertions in tests outside `AssertState.rootpath` (#13403).""" | ||
pytester.makepyfile( | ||
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 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
Then execute If I understand the objective of the issue correctly:
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 commentThe 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 |
||
**{ | ||
"file.py": """\ | ||
def test_simple_failure(): | ||
assert 1 + 1 == 3 | ||
""" | ||
} | ||
) | ||
with mock.patch.object(hook, "fnpats", ["*.py"]): | ||
assert hook.find_spec("file") is not None | ||
|
||
rootpath = f"{os.getcwd()}/tests" | ||
if not os.path.exists(rootpath): | ||
mkdir(rootpath) | ||
monkeypatch.setattr( | ||
pytester._request.config, | ||
"invocation_params", | ||
Config.InvocationParams( | ||
args=(), | ||
plugins=(), | ||
dir=Path(rootpath), | ||
), | ||
) | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. mamy name it in another way - test_assert_rewrite_for_conftfest 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. My previous suggestion about using a real-case scenario makes this test unnecessary. |
||
self, pytester: Pytester, hook: AssertionRewritingHook, monkeypatch | ||
) -> None: | ||
"""Conftest is always rewritten regardless of the root dir""" | ||
pytester.makeconftest( | ||
""" | ||
import pytest | ||
@pytest.fixture | ||
def fix(): return 1 | ||
""" | ||
) | ||
|
||
rootpath = f"{os.getcwd()}/tests" | ||
if not os.path.exists(rootpath): | ||
mkdir(rootpath) | ||
monkeypatch.setattr( | ||
pytester._request.config, | ||
"invocation_params", | ||
Config.InvocationParams( | ||
args=(), | ||
plugins=(), | ||
dir=Path(rootpath), | ||
), | ||
) | ||
with mock.patch.object(hook, "fnpats", ["*.py"]): | ||
assert hook.find_spec("conftest") is not None | ||
|
||
def test_assert_rewrite_correct_for_plugins( | ||
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. My previous suggestion about using a real-case scenario makes this test unnecessary. |
||
self, pytester: Pytester, hook: AssertionRewritingHook, monkeypatch | ||
) -> None: | ||
""" | ||
Plugins has always been rewritten regardless of the root dir | ||
""" | ||
pkgdir = pytester.mkpydir("plugin") | ||
pkgdir.joinpath("__init__.py").write_text( | ||
"import pytest\n" | ||
"@pytest.fixture\n" | ||
"def special_asserter():\n" | ||
" def special_assert(x, y):\n" | ||
" assert x == y\n" | ||
" return special_assert\n", | ||
encoding="utf-8", | ||
) | ||
hook.mark_rewrite("plugin") | ||
rootpath = f"{os.getcwd()}/tests" | ||
if not os.path.exists(rootpath): | ||
mkdir(rootpath) | ||
monkeypatch.setattr( | ||
pytester._request.config, | ||
"invocation_params", | ||
Config.InvocationParams( | ||
args=(), | ||
plugins=(), | ||
dir=Path(rootpath), | ||
), | ||
) | ||
with mock.patch.object(hook, "fnpats", ["*.py"]): | ||
assert hook.find_spec("plugin") is not None | ||
|
||
@pytest.mark.skipif( | ||
sys.platform.startswith("win32"), reason="cannot remove cwd on Windows" | ||
) | ||
|
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.
Can we elaborate a bit on "external modules" here? This should be written for users scanning the changelog.