-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix assert rewriting with assignment expressions #11414
Changes from 1 commit
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 @@ | ||
Fixed ``:=`` in asserts impacting unrelated test cases. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||
import sys | ||||||||||||||||||||||||||||||||||||||||||||||
import tokenize | ||||||||||||||||||||||||||||||||||||||||||||||
import types | ||||||||||||||||||||||||||||||||||||||||||||||
from collections import defaultdict | ||||||||||||||||||||||||||||||||||||||||||||||
from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||
from pathlib import PurePath | ||||||||||||||||||||||||||||||||||||||||||||||
from typing import Callable | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -52,6 +53,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||
PYC_EXT = ".py" + (__debug__ and "c" or "o") | ||||||||||||||||||||||||||||||||||||||||||||||
PYC_TAIL = "." + PYTEST_TAG + PYC_EXT | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
_SCOPE_END_MARKER = object() | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
class AssertionRewritingHook(importlib.abc.MetaPathFinder, importlib.abc.Loader): | ||||||||||||||||||||||||||||||||||||||||||||||
"""PEP302/PEP451 import hook which rewrites asserts.""" | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -634,6 +637,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||
.push_format_context() and .pop_format_context() which allows | ||||||||||||||||||||||||||||||||||||||||||||||
to build another %-formatted string while already building one. | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
:scope: A tuple containing the current scope used for variables_overwrite. | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
:variables_overwrite: A dict filled with references to variables | ||||||||||||||||||||||||||||||||||||||||||||||
that change value within an assert. This happens when a variable is | ||||||||||||||||||||||||||||||||||||||||||||||
reassigned with the walrus operator | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -655,7 +660,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||||||
self.enable_assertion_pass_hook = False | ||||||||||||||||||||||||||||||||||||||||||||||
self.source = source | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite: Dict[str, str] = {} | ||||||||||||||||||||||||||||||||||||||||||||||
self.scope: tuple[ast.AST, ...] = () | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite: defaultdict[ | ||||||||||||||||||||||||||||||||||||||||||||||
tuple[ast.AST, ...], Dict[str, str] | ||||||||||||||||||||||||||||||||||||||||||||||
] = defaultdict(dict) | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
def run(self, mod: ast.Module) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||
"""Find all assert statements in *mod* and rewrite them.""" | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -719,9 +727,17 @@ | |||||||||||||||||||||||||||||||||||||||||||||
mod.body[pos:pos] = imports | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
# Collect asserts. | ||||||||||||||||||||||||||||||||||||||||||||||
nodes: List[ast.AST] = [mod] | ||||||||||||||||||||||||||||||||||||||||||||||
self.scope = (mod,) | ||||||||||||||||||||||||||||||||||||||||||||||
nodes: List[Union[ast.AST, object]] = [mod] | ||||||||||||||||||||||||||||||||||||||||||||||
while nodes: | ||||||||||||||||||||||||||||||||||||||||||||||
node = nodes.pop() | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): | ||||||||||||||||||||||||||||||||||||||||||||||
self.scope = tuple((*self.scope, node)) | ||||||||||||||||||||||||||||||||||||||||||||||
nodes.append(_SCOPE_END_MARKER) | ||||||||||||||||||||||||||||||||||||||||||||||
if node == _SCOPE_END_MARKER: | ||||||||||||||||||||||||||||||||||||||||||||||
self.scope = self.scope[:-1] | ||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+741
to
+743
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 don't quite understand the role of 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. Although the An example
There are a few caveats, mainly that not all child nodes are actually in the child scope (e.g. default arguments which are evaluated in the parent scope), but it doesn't really matter here as those don't usually contain Hope that at least somewhat makes sense. pytest/src/_pytest/assertion/rewrite.py Lines 722 to 743 in e5c81fa
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! |
||||||||||||||||||||||||||||||||||||||||||||||
continue | ||||||||||||||||||||||||||||||||||||||||||||||
assert isinstance(node, ast.AST) | ||||||||||||||||||||||||||||||||||||||||||||||
for name, field in ast.iter_fields(node): | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance(field, list): | ||||||||||||||||||||||||||||||||||||||||||||||
new: List[ast.AST] = [] | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -992,7 +1008,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||||||
pytest_temp = self.variable() | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite[ | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite[self.scope][ | ||||||||||||||||||||||||||||||||||||||||||||||
v.left.target.id | ||||||||||||||||||||||||||||||||||||||||||||||
] = v.left # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
v.left.target.id = pytest_temp | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -1035,17 +1051,20 @@ | |||||||||||||||||||||||||||||||||||||||||||||
new_args = [] | ||||||||||||||||||||||||||||||||||||||||||||||
new_kwargs = [] | ||||||||||||||||||||||||||||||||||||||||||||||
for arg in call.args: | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance(arg, ast.Name) and arg.id in self.variables_overwrite: | ||||||||||||||||||||||||||||||||||||||||||||||
arg = self.variables_overwrite[arg.id] # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance(arg, ast.Name) and arg.id in self.variables_overwrite.get( | ||||||||||||||||||||||||||||||||||||||||||||||
self.scope, {} | ||||||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||||||
arg = self.variables_overwrite[self.scope][ | ||||||||||||||||||||||||||||||||||||||||||||||
arg.id | ||||||||||||||||||||||||||||||||||||||||||||||
] # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
res, expl = self.visit(arg) | ||||||||||||||||||||||||||||||||||||||||||||||
arg_expls.append(expl) | ||||||||||||||||||||||||||||||||||||||||||||||
new_args.append(res) | ||||||||||||||||||||||||||||||||||||||||||||||
for keyword in call.keywords: | ||||||||||||||||||||||||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||||||||||||||||||||||||
isinstance(keyword.value, ast.Name) | ||||||||||||||||||||||||||||||||||||||||||||||
and keyword.value.id in self.variables_overwrite | ||||||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||||||
keyword.value = self.variables_overwrite[ | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance( | ||||||||||||||||||||||||||||||||||||||||||||||
keyword.value, ast.Name | ||||||||||||||||||||||||||||||||||||||||||||||
) and keyword.value.id in self.variables_overwrite.get(self.scope, {}): | ||||||||||||||||||||||||||||||||||||||||||||||
keyword.value = self.variables_overwrite[self.scope][ | ||||||||||||||||||||||||||||||||||||||||||||||
keyword.value.id | ||||||||||||||||||||||||||||||||||||||||||||||
] # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
res, expl = self.visit(keyword.value) | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -1081,12 +1100,14 @@ | |||||||||||||||||||||||||||||||||||||||||||||
def visit_Compare(self, comp: ast.Compare) -> Tuple[ast.expr, str]: | ||||||||||||||||||||||||||||||||||||||||||||||
self.push_format_context() | ||||||||||||||||||||||||||||||||||||||||||||||
# We first check if we have overwritten a variable in the previous assert | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance(comp.left, ast.Name) and comp.left.id in self.variables_overwrite: | ||||||||||||||||||||||||||||||||||||||||||||||
comp.left = self.variables_overwrite[ | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance( | ||||||||||||||||||||||||||||||||||||||||||||||
comp.left, ast.Name | ||||||||||||||||||||||||||||||||||||||||||||||
) and comp.left.id in self.variables_overwrite.get(self.scope, {}): | ||||||||||||||||||||||||||||||||||||||||||||||
comp.left = self.variables_overwrite[self.scope][ | ||||||||||||||||||||||||||||||||||||||||||||||
comp.left.id | ||||||||||||||||||||||||||||||||||||||||||||||
] # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
if isinstance(comp.left, ast.NamedExpr): | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite[ | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite[self.scope][ | ||||||||||||||||||||||||||||||||||||||||||||||
comp.left.target.id | ||||||||||||||||||||||||||||||||||||||||||||||
] = comp.left # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
left_res, left_expl = self.visit(comp.left) | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -1106,7 +1127,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||
and next_operand.target.id == left_res.id | ||||||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||||||
next_operand.target.id = self.variable() | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite[ | ||||||||||||||||||||||||||||||||||||||||||||||
self.variables_overwrite[self.scope][ | ||||||||||||||||||||||||||||||||||||||||||||||
left_res.id | ||||||||||||||||||||||||||||||||||||||||||||||
] = next_operand # type:ignore[assignment] | ||||||||||||||||||||||||||||||||||||||||||||||
next_res, next_expl = self.visit(next_operand) | ||||||||||||||||||||||||||||||||||||||||||||||
|
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.
Minor, but how about we make a specific subclass from
ast.AST
, so the typing forscope: tuple[ast.AST, ...] = ()
is consistent?And then instead of checking
if node == _SCOPE_END_MARKER:
. we canif isinstance(node, ScopeEnd)
.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
self.scope
typing is correct withtuple[ast.AST, ...]
already._SCOE_END_MARKER
is only added to thenodes
list never toself.scope
.