feat(rewrite): address assertion rewriting blind spots with systematic tests#139
Open
RonnyPfannschmidt wants to merge 8 commits into
Open
feat(rewrite): address assertion rewriting blind spots with systematic tests#139RonnyPfannschmidt wants to merge 8 commits into
RonnyPfannschmidt wants to merge 8 commits into
Conversation
Create testing/test_assertrewrite_coverage.py with reusable helpers for verifying assertion rewriting behavior across all expression types: - get_failure_message: compile rewritten source and extract failure text - assert_introspects: verify failure messages contain expected intermediates - assert_single_evaluation: verify no double-evaluation of side effects - assert_passes_when_true: verify rewritten asserts don't false-positive - assert_semantically_equivalent: verify rewrite preserves pass/fail semantics Includes smoke tests validating the helpers themselves. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Add parametrized test classes that document current assertion rewriting behavior for each AST expression type: - Compare, BoolOp, UnaryOp, BinOp: verified working with correct output - Call: works but shows <function ...> repr for local/variable callables - Attribute, Name, Walrus: verified working - Subscript, IfExp, ContainerLiteral: marked xfail (blind spots) - MethodCall: shows noisy bound-method intermediate (xfail) - Comprehension, FString: semantics preserved, result shown in compare The xfail tests document expected improvements and will be unmarked as each blind spot is addressed in subsequent commits. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Add TestSingleEvaluation class that systematically verifies no expression is evaluated multiple times during assertion rewriting. Covers: - Calls in compare, boolean, unary, binop contexts - Attribute/property access - Subscript (dict __getitem__) - Walrus operator in compare, boolean, and chained compare - Method calls - Nested calls (inner + outer counted separately) - Multiple comparators in chained comparisons - IfExp conditions - Comprehension generators All tests pass, confirming the pytest-dev#14445 fix holds across all expression types. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Implement a dedicated visitor for ast.Subscript in AssertionRewriter
that decomposes container[key] expressions into separate container and
key introspection. This produces failure messages like:
assert 1 == 99
+ where 1 = {'a': 1, 'b': 2}['a']
Previously, subscript expressions hit generic_visit and only showed
the final value without decomposition into container and key.
Slices (a[1:3]) still fall back to generic_visit since decomposing
start/stop/step is rarely useful in assertion messages.
Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Implement a dedicated visitor for ast.IfExp that introspects the
condition value while preserving short-circuit semantics. Produces
failure messages like:
assert 0 == 99
+ where 0 = (... if True else ...)
The condition is rewritten for introspection (showing its evaluated
value), but branches are kept as-is to preserve Python's short-circuit
behavior — only the selected branch is evaluated.
Previously, IfExp hit generic_visit showing only the final result
without any insight into which branch was taken or why.
Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Refactor visit_Call to detect obj.method() patterns (where call.func is
an ast.Attribute) and produce a flat explanation format:
assert 42 == 100
+ where 42 = Obj().compute()
Instead of the previous nested format:
assert 42 == 100
+ where 42 = compute()
+ where compute = Obj().compute
The bound-method intermediate line was noisy and unhelpful — users want
to see what object the method was called on and what it returned, not
the method object itself.
Regular function calls (non-attribute) are unchanged.
Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Add TestEdgeCases class combining the new visitors (Subscript, IfExp, method call) with existing ones to verify correct behavior in complex scenarios: - Subscript with variable keys, call keys, and nested subscripts - Method calls with arguments, chained calls, and global objects - IfExp with call conditions - Walrus operator in subscript keys - Single-evaluation guarantees for all new visitors - Custom assert messages still work with new decomposition - Complex assertions combining multiple visitor types Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
Reviewer's GuideExtends pytest’s assertion rewriting with targeted handling for method calls, subscripts, and ternary if-expressions, and adds a large systematic test suite to exercise coverage, semantics, and single-evaluation guarantees across many expression forms. Sequence diagram for flattened method call assertion rewritingsequenceDiagram
participant TestCode
participant AssertionRewriter
participant AST_Call as ast_Call
participant AST_Attr as ast_Attribute
TestCode->>AssertionRewriter: assert Obj().compute(x)
AssertionRewriter->>AssertionRewriter: visit_Call(call)
AssertionRewriter->>AST_Call: inspect func
AST_Call-->>AssertionRewriter: func is ast_Attribute (Load)
AssertionRewriter->>AssertionRewriter: _visit_method_call(call)
Note over AssertionRewriter: Visit receiver object
AssertionRewriter->>AssertionRewriter: visit(attr.value)
AssertionRewriter-->>AssertionRewriter: obj_res, obj_expl
Note over AssertionRewriter: Visit positional args
loop for each arg in call.args
AssertionRewriter->>AssertionRewriter: visit(arg)
AssertionRewriter-->>AssertionRewriter: res, expl
end
Note over AssertionRewriter: Visit keyword args
loop for each kw in call.keywords
AssertionRewriter->>AssertionRewriter: visit(keyword.value)
AssertionRewriter-->>AssertionRewriter: res, expl
end
Note over AssertionRewriter: Build new call on rewritten receiver
AssertionRewriter->>AST_Attr: create Attribute(obj_res, attr.attr, Load)
AssertionRewriter->>AssertionRewriter: assign(new_call) -> res
AssertionRewriter->>AssertionRewriter: explanation_param(display(res)) -> res_expl
Note over AssertionRewriter: Compose flat explanation
AssertionRewriter-->>TestCode: where res_expl = obj_expl.attr(args)
Class diagram for updated assertion rewriting visitorsclassDiagram
class AssertionRewriter {
+visit_Call(call)
+visit_IfExp(ifexp)
+visit_Subscript(subscript)
+visit_Attribute(attr)
-_visit_method_call(call)
+visit(node)
+assign(node)
+explanation_param(text)
+display(node)
+generic_visit(node)
}
class ast_Call {
+func
+args
+keywords
}
class ast_Attribute {
+value
+attr
+ctx
}
class ast_IfExp {
+test
+body
+orelse
}
class ast_Subscript {
+value
+slice
+ctx
}
class ast_Slice {
+lower
+upper
+step
}
AssertionRewriter ..> ast_Call : visits
AssertionRewriter ..> ast_Attribute : visits
AssertionRewriter ..> ast_IfExp : visits
AssertionRewriter ..> ast_Subscript : visits
AssertionRewriter ..> ast_Slice : checks
AssertionRewriter --> AssertionRewriter : _visit_method_call used by visit_Call
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
_visit_method_call, usingobj_expldirectly in the explanation ({obj_expl}.{attr.attr}(...) will embed the full nested explanation (including braces/newlines) instead of the object’s display value; consider usingself.display(obj_res)(or similar) there to keep the method call line flat and readable as intended. visit_IfExpreconstructs theIfExpwith an unvisitedbodyandorelse, which avoids double evaluation but also suppresses any introspection or rewriting inside those branches; it might be worth explicitly documenting this trade-off in-code and/or asserting that important node types inside branches are still handled where necessary.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_visit_method_call`, using `obj_expl` directly in the explanation (`{obj_expl}.{attr.attr}(...`) will embed the full nested explanation (including braces/newlines) instead of the object’s display value; consider using `self.display(obj_res)` (or similar) there to keep the method call line flat and readable as intended.
- `visit_IfExp` reconstructs the `IfExp` with an unvisited `body` and `orelse`, which avoids double evaluation but also suppresses any introspection or rewriting inside those branches; it might be worth explicitly documenting this trade-off in-code and/or asserting that important node types inside branches are still handled where necessary.
## Individual Comments
### Comment 1
<location path="src/_pytest/assertion/rewrite.py" line_range="1029-1030" />
<code_context>
+ # For method calls (obj.method()), produce a flat explanation like
+ # "where result = obj.method(args)" instead of nesting the attribute
+ # access as a separate "where method = obj.method" line.
+ if isinstance(call.func, ast.Attribute) and isinstance(call.func.ctx, ast.Load):
+ return self._visit_method_call(call)
+
new_func, func_expl = self.visit(call.func)
</code_context>
<issue_to_address>
**suggestion:** Method-call special case duplicates `visit_Call` logic and risks future divergence.
The early return to `_visit_method_call` re-implements arg/keyword handling already in `visit_Call`, so these paths can easily diverge if `visit_Call` changes (e.g., for new AST shapes), causing inconsistent explanations between `obj.method(...)` and `func(...)`. It’d be better to extract the shared arg/kw visiting and call construction into a helper used by both `visit_Call` and `_visit_method_call`, keeping only the explanation formatting separate.
Suggested implementation:
```python
# For method calls (obj.method()), produce a flat explanation like
# "where result = obj.method(args)" instead of nesting the attribute
# access as a separate "where method = obj.method" line.
if isinstance(call.func, ast.Attribute) and isinstance(call.func.ctx, ast.Load):
return self._visit_method_call(call)
new_func, func_expl = self.visit(call.func)
new_args, new_keywords, arg_expls = self._visit_call_arguments(call)
outer_expl = f"{res_expl}\n{{{res_expl} = {expl}\n}}"
return res, outer_expl
def _visit_call_arguments(self, call: ast.Call) -> tuple[list[ast.expr], list[ast.keyword], list[str]]:
"""Visit and rewrite arguments/keywords for a Call node.
Returns:
(new_args, new_keywords, arg_explanations)
"""
arg_expls: list[str] = []
new_args: list[ast.expr] = []
for arg in call.args:
new_arg, arg_expl = self.visit(arg)
new_args.append(new_arg)
if arg_expl:
arg_expls.append(arg_expl)
new_keywords: list[ast.keyword] = []
for kw in call.keywords:
new_value, kw_expl = self.visit(kw.value)
new_keywords.append(ast.keyword(arg=kw.arg, value=new_value))
if kw_expl:
arg_expls.append(kw_expl)
return new_args, new_keywords, arg_expls
def _visit_method_call(self, call: ast.Call) -> tuple[ast.Name, str]:
```
To fully apply the refactoring and avoid duplication/divergence you should also:
1. Inside `_visit_method_call`, replace any inlined loops or logic that visit `call.args` / `call.keywords` with a call to `_visit_call_arguments(call)`, e.g.:
```python
new_args, new_keywords, arg_expls = self._visit_call_arguments(call)
```
and then use `new_args`, `new_keywords`, and `arg_expls` in the method-call-specific explanation formatting.
2. Ensure that wherever `visit_Call` previously constructed the new `ast.Call` node and explanation using `arg_expls` / `new_args` / `call.keywords`, it now uses the `new_keywords` returned from `_visit_call_arguments`.
3. If this file does not already import or use `list[...]` type annotations elsewhere, you may want to either:
- Add `from __future__ import annotations` at the top of the file, or
- Replace the `tuple[list[ast.expr], ...]` annotations with `Tuple[List[ast.expr], ...]` and import `Tuple, List` from `typing`, to match the existing style.
These updates will keep the argument/keyword handling logic shared between `visit_Call` and `_visit_method_call`, while allowing `_visit_method_call` to maintain its special, flat explanation format.
</issue_to_address>
### Comment 2
<location path="src/_pytest/assertion/rewrite.py" line_range="1089" />
<code_context>
+ res = self.assign(new_call)
+ res_expl = self.explanation_param(self.display(res))
+ args_str = ", ".join(arg_expls)
+ expl = f"{res_expl}\n{{{res_expl} = {obj_expl}.{attr.attr}({args_str})\n}}"
+ return res, expl
+
</code_context>
<issue_to_address>
**issue:** Using `obj_expl` inside the call expression can produce confusing, nested explanations.
Because `obj_expl` is often a multi-line block (e.g. `obj_repr\n{obj_repr = ...}`), embedding it directly in the call (`{obj_expl}.{attr.attr}(...)`) will produce awkward, hard-to-read output like `result = obj\n{obj = ...}.method(...)` and can conflict with the existing braced formatting. Consider using the displayed value (`self.display(res)` wrapped with `explanation_param`) in the call expression, and reserving `obj_expl` for a separate `where`/detail line instead.
</issue_to_address>
### Comment 3
<location path="testing/test_assertrewrite_coverage.py" line_range="536-537" />
<code_context>
+ )
+
+
+class TestIntrospectionSubscript:
+ """Subscript / indexing — now has dedicated visitor."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for slice subscripts to lock in `visit_Subscript` fallback behavior and evaluation count
The new `visit_Subscript` only has coverage for simple index subscripts; it doesn’t exercise the `ast.Slice` path via `generic_visit`. Please add:
* A semantic test, e.g.:
```python
def test_slice_semantics_preserved(self) -> None:
assert_semantically_equivalent(
'''
def check():
xs = [0, 1, 2, 3]
assert xs[1:3] == [0]
'''
)
```
* A single-evaluation test where container or bounds have side effects (e.g. `xs()[start():end()]`) to confirm the fallback path preserves evaluation counts.
This will help catch regressions if `visit_Subscript`’s slice handling changes in future.
```suggestion
class TestIntrospectionSubscript:
"""Subscript / indexing — now has dedicated visitor."""
def test_slice_semantics_preserved(self) -> None:
assert_semantically_equivalent(
"""
def check():
xs = [0, 1, 2, 3]
# Ensure slice semantics (bounds, step, and result) are preserved
assert xs[1:3] == [1, 2]
"""
)
def test_slice_single_evaluation(self) -> None:
# Container and slice bounds all have side effects; they must each
# be evaluated exactly once even when going through generic_visit
# for ast.Slice in visit_Subscript.
assert_single_evaluation(
"""
calls = []
def mk_list():
calls.append("mk_list")
return [0, 1, 2, 3]
def start():
calls.append("start")
return 1
def end():
calls.append("end")
return 3
def check():
global calls
calls = []
assert mk_list()[start():end()] == [1, 2]
# Container and each bound evaluated exactly once
assert calls == ["mk_list", "start", "end"]
"""
)
```
</issue_to_address>
### Comment 4
<location path="testing/test_assertrewrite_coverage.py" line_range="996" />
<code_context>
+ must_contain=["42", "100"],
+ )
+
+ def test_method_call_with_args(self) -> None:
+ """Method call with arguments shows flat format."""
+ assert_introspects(
</code_context>
<issue_to_address>
**suggestion (testing):** Extend method-call tests to cover keyword and **kwargs arguments with the flattened format
Right now this only covers positional args with the flattened `Obj().method()` format, but `_visit_method_call` also handles keyword and `**` arguments. Please add tests that exercise those paths, e.g. a method with `self, a, b=0, **kw` and a call like `C().f(1, b=2, extra=3)` asserting that the introspection output includes `where 6 = C().f(1, b=2, extra=3)`. It would also be useful to add an `assert_single_evaluation` case where keyword values or the `**` mapping have side effects, to confirm we don’t introduce extra evaluations.
Suggested implementation:
```python
"""Nested subscript: d[k1][k2]."""
assert_introspects(
"""
def check():
d = {"a": {"b": 42}}
assert d["a"]["b"] == 100
""",
must_contain=["42", "100"],
)
def test_method_call_with_args(self) -> None:
"""Method call with positional, keyword, and **kwargs shows flat format."""
assert_introspects(
"""
class C:
def f(self, a, b=0, **kw):
return a + b + sum(kw.values())
def check():
assert C().f(1, b=2, extra=3) == 6
""",
must_contain=["where 6 = C().f(1, b=2, extra=3)"],
)
def test_method_call_with_kwargs_single_evaluation(self) -> None:
"""Keyword and **kwargs arguments are only evaluated once in method calls."""
assert_single_evaluation(
"""
class SideEffects:
def __init__(self):
self.calls = []
def arg(self, name, value):
self.calls.append(name)
return value
def mapping(self):
self.calls.append("mapping")
return {"b": self.arg("b", 2), "extra": self.arg("extra", 3)}
class C:
def f(self, a, b=0, **kw):
return a + b + sum(kw.values())
def check():
s = SideEffects()
assert C().f(s.arg("a", 1), **s.mapping()) == 6
assert s.calls == ["a", "mapping", "b", "extra"]
""",
expected_evaluations={"SideEffects.arg": 3, "SideEffects.mapping": 1},
)
def test_assert_introspects_fails_on_missing(self) -> None:
with pytest.raises(AssertionError, match=r"Expected.*in failure"):
```
The new `assert_single_evaluation` call assumes an API of:
```python
assert_single_evaluation(code: str, expected_evaluations: dict[str, int]) -> None
```
If the existing helper has a different signature or assertion style, adjust the `expected_evaluations` argument accordingly (for example, it might take a mapping of expression strings to counts, or separate parameters for the code and the symbol to track). The important behavioral checks are:
1. The introspected expression in `test_method_call_with_args` includes the flattened `where 6 = C().f(1, b=2, extra=3)` line.
2. In `test_method_call_with_kwargs_single_evaluation`, `SideEffects.arg` is evaluated exactly three times (for `"a"`, `"b"`, `"extra"`) and `SideEffects.mapping` exactly once, with no duplicate evaluations introduced by introspection.
</issue_to_address>
### Comment 5
<location path="testing/test_assertrewrite_coverage.py" line_range="886-893" />
<code_context>
+ assert 1 < (x := side_effect()) < 3
+""")
+
+ def test_method_call_evaluated_once(self) -> None:
+ assert_single_evaluation("""
+def check():
</code_context>
<issue_to_address>
**suggestion (testing):** Also cover method calls where the receiver expression itself has side effects
Current single-evaluation tests for method calls cover side effects inside the method body. To fully exercise `_visit_method_call`, please also add a case where the receiver expression has side effects, e.g. `factory().compute()`, and assert that the receiver is evaluated only once. This will verify that rewriting `call.func` from an `Attribute` on a temporary name still preserves the single-evaluation guarantee for the receiver expression.
```suggestion
def test_walrus_in_chained_compare_evaluated_once(self) -> None:
assert_single_evaluation("""
def check():
def side_effect():
counter[0] += 1
return 5
assert 1 < (x := side_effect()) < 3
""")
def test_method_call_receiver_evaluated_once(self) -> None:
assert_single_evaluation("""
def check():
class C:
def compute(self):
return 1
def factory():
counter[0] += 1
return C()
assert factory().compute() == 1
""")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1029
to
+1030
| if isinstance(call.func, ast.Attribute) and isinstance(call.func.ctx, ast.Load): | ||
| return self._visit_method_call(call) |
There was a problem hiding this comment.
suggestion: Method-call special case duplicates visit_Call logic and risks future divergence.
The early return to _visit_method_call re-implements arg/keyword handling already in visit_Call, so these paths can easily diverge if visit_Call changes (e.g., for new AST shapes), causing inconsistent explanations between obj.method(...) and func(...). It’d be better to extract the shared arg/kw visiting and call construction into a helper used by both visit_Call and _visit_method_call, keeping only the explanation formatting separate.
Suggested implementation:
# For method calls (obj.method()), produce a flat explanation like
# "where result = obj.method(args)" instead of nesting the attribute
# access as a separate "where method = obj.method" line.
if isinstance(call.func, ast.Attribute) and isinstance(call.func.ctx, ast.Load):
return self._visit_method_call(call)
new_func, func_expl = self.visit(call.func)
new_args, new_keywords, arg_expls = self._visit_call_arguments(call)
outer_expl = f"{res_expl}\n{{{res_expl} = {expl}\n}}"
return res, outer_expl
def _visit_call_arguments(self, call: ast.Call) -> tuple[list[ast.expr], list[ast.keyword], list[str]]:
"""Visit and rewrite arguments/keywords for a Call node.
Returns:
(new_args, new_keywords, arg_explanations)
"""
arg_expls: list[str] = []
new_args: list[ast.expr] = []
for arg in call.args:
new_arg, arg_expl = self.visit(arg)
new_args.append(new_arg)
if arg_expl:
arg_expls.append(arg_expl)
new_keywords: list[ast.keyword] = []
for kw in call.keywords:
new_value, kw_expl = self.visit(kw.value)
new_keywords.append(ast.keyword(arg=kw.arg, value=new_value))
if kw_expl:
arg_expls.append(kw_expl)
return new_args, new_keywords, arg_expls
def _visit_method_call(self, call: ast.Call) -> tuple[ast.Name, str]:To fully apply the refactoring and avoid duplication/divergence you should also:
-
Inside
_visit_method_call, replace any inlined loops or logic that visitcall.args/call.keywordswith a call to_visit_call_arguments(call), e.g.:new_args, new_keywords, arg_expls = self._visit_call_arguments(call)
and then use
new_args,new_keywords, andarg_explsin the method-call-specific explanation formatting. -
Ensure that wherever
visit_Callpreviously constructed the newast.Callnode and explanation usingarg_expls/new_args/call.keywords, it now uses thenew_keywordsreturned from_visit_call_arguments. -
If this file does not already import or use
list[...]type annotations elsewhere, you may want to either:- Add
from __future__ import annotationsat the top of the file, or - Replace the
tuple[list[ast.expr], ...]annotations withTuple[List[ast.expr], ...]and importTuple, Listfromtyping, to match the existing style.
- Add
These updates will keep the argument/keyword handling logic shared between visit_Call and _visit_method_call, while allowing _visit_method_call to maintain its special, flat explanation format.
| res = self.assign(new_call) | ||
| res_expl = self.explanation_param(self.display(res)) | ||
| args_str = ", ".join(arg_expls) | ||
| expl = f"{res_expl}\n{{{res_expl} = {obj_expl}.{attr.attr}({args_str})\n}}" |
There was a problem hiding this comment.
issue: Using obj_expl inside the call expression can produce confusing, nested explanations.
Because obj_expl is often a multi-line block (e.g. obj_repr\n{obj_repr = ...}), embedding it directly in the call ({obj_expl}.{attr.attr}(...)) will produce awkward, hard-to-read output like result = obj\n{obj = ...}.method(...) and can conflict with the existing braced formatting. Consider using the displayed value (self.display(res) wrapped with explanation_param) in the call expression, and reserving obj_expl for a separate where/detail line instead.
Comment on lines
+536
to
+537
| class TestIntrospectionSubscript: | ||
| """Subscript / indexing — now has dedicated visitor.""" |
There was a problem hiding this comment.
suggestion (testing): Add explicit tests for slice subscripts to lock in visit_Subscript fallback behavior and evaluation count
The new visit_Subscript only has coverage for simple index subscripts; it doesn’t exercise the ast.Slice path via generic_visit. Please add:
- A semantic test, e.g.:
def test_slice_semantics_preserved(self) -> None: assert_semantically_equivalent( ''' def check(): xs = [0, 1, 2, 3] assert xs[1:3] == [0] ''' )
- A single-evaluation test where container or bounds have side effects (e.g.
xs()[start():end()]) to confirm the fallback path preserves evaluation counts.
This will help catch regressions if visit_Subscript’s slice handling changes in future.
Suggested change
| class TestIntrospectionSubscript: | |
| """Subscript / indexing — now has dedicated visitor.""" | |
| class TestIntrospectionSubscript: | |
| """Subscript / indexing — now has dedicated visitor.""" | |
| def test_slice_semantics_preserved(self) -> None: | |
| assert_semantically_equivalent( | |
| """ | |
| def check(): | |
| xs = [0, 1, 2, 3] | |
| # Ensure slice semantics (bounds, step, and result) are preserved | |
| assert xs[1:3] == [1, 2] | |
| """ | |
| ) | |
| def test_slice_single_evaluation(self) -> None: | |
| # Container and slice bounds all have side effects; they must each | |
| # be evaluated exactly once even when going through generic_visit | |
| # for ast.Slice in visit_Subscript. | |
| assert_single_evaluation( | |
| """ | |
| calls = [] | |
| def mk_list(): | |
| calls.append("mk_list") | |
| return [0, 1, 2, 3] | |
| def start(): | |
| calls.append("start") | |
| return 1 | |
| def end(): | |
| calls.append("end") | |
| return 3 | |
| def check(): | |
| global calls | |
| calls = [] | |
| assert mk_list()[start():end()] == [1, 2] | |
| # Container and each bound evaluated exactly once | |
| assert calls == ["mk_list", "start", "end"] | |
| """ | |
| ) |
| must_contain=["42", "100"], | ||
| ) | ||
|
|
||
| def test_method_call_with_args(self) -> None: |
There was a problem hiding this comment.
suggestion (testing): Extend method-call tests to cover keyword and **kwargs arguments with the flattened format
Right now this only covers positional args with the flattened Obj().method() format, but _visit_method_call also handles keyword and ** arguments. Please add tests that exercise those paths, e.g. a method with self, a, b=0, **kw and a call like C().f(1, b=2, extra=3) asserting that the introspection output includes where 6 = C().f(1, b=2, extra=3). It would also be useful to add an assert_single_evaluation case where keyword values or the ** mapping have side effects, to confirm we don’t introduce extra evaluations.
Suggested implementation:
"""Nested subscript: d[k1][k2]."""
assert_introspects(
"""
def check():
d = {"a": {"b": 42}}
assert d["a"]["b"] == 100
""",
must_contain=["42", "100"],
)
def test_method_call_with_args(self) -> None:
"""Method call with positional, keyword, and **kwargs shows flat format."""
assert_introspects(
"""
class C:
def f(self, a, b=0, **kw):
return a + b + sum(kw.values())
def check():
assert C().f(1, b=2, extra=3) == 6
""",
must_contain=["where 6 = C().f(1, b=2, extra=3)"],
)
def test_method_call_with_kwargs_single_evaluation(self) -> None:
"""Keyword and **kwargs arguments are only evaluated once in method calls."""
assert_single_evaluation(
"""
class SideEffects:
def __init__(self):
self.calls = []
def arg(self, name, value):
self.calls.append(name)
return value
def mapping(self):
self.calls.append("mapping")
return {"b": self.arg("b", 2), "extra": self.arg("extra", 3)}
class C:
def f(self, a, b=0, **kw):
return a + b + sum(kw.values())
def check():
s = SideEffects()
assert C().f(s.arg("a", 1), **s.mapping()) == 6
assert s.calls == ["a", "mapping", "b", "extra"]
""",
expected_evaluations={"SideEffects.arg": 3, "SideEffects.mapping": 1},
)
def test_assert_introspects_fails_on_missing(self) -> None:
with pytest.raises(AssertionError, match=r"Expected.*in failure"):The new assert_single_evaluation call assumes an API of:
assert_single_evaluation(code: str, expected_evaluations: dict[str, int]) -> NoneIf the existing helper has a different signature or assertion style, adjust the expected_evaluations argument accordingly (for example, it might take a mapping of expression strings to counts, or separate parameters for the code and the symbol to track). The important behavioral checks are:
- The introspected expression in
test_method_call_with_argsincludes the flattenedwhere 6 = C().f(1, b=2, extra=3)line. - In
test_method_call_with_kwargs_single_evaluation,SideEffects.argis evaluated exactly three times (for"a","b","extra") andSideEffects.mappingexactly once, with no duplicate evaluations introduced by introspection.
Comment on lines
+886
to
+893
| def test_walrus_in_chained_compare_evaluated_once(self) -> None: | ||
| assert_single_evaluation(""" | ||
| def check(): | ||
| def side_effect(): | ||
| counter[0] += 1 | ||
| return 5 | ||
| assert 1 < (x := side_effect()) < 3 | ||
| """) |
There was a problem hiding this comment.
suggestion (testing): Also cover method calls where the receiver expression itself has side effects
Current single-evaluation tests for method calls cover side effects inside the method body. To fully exercise _visit_method_call, please also add a case where the receiver expression has side effects, e.g. factory().compute(), and assert that the receiver is evaluated only once. This will verify that rewriting call.func from an Attribute on a temporary name still preserves the single-evaluation guarantee for the receiver expression.
Suggested change
| def test_walrus_in_chained_compare_evaluated_once(self) -> None: | |
| assert_single_evaluation(""" | |
| def check(): | |
| def side_effect(): | |
| counter[0] += 1 | |
| return 5 | |
| assert 1 < (x := side_effect()) < 3 | |
| """) | |
| def test_walrus_in_chained_compare_evaluated_once(self) -> None: | |
| assert_single_evaluation(""" | |
| def check(): | |
| def side_effect(): | |
| counter[0] += 1 | |
| return 5 | |
| assert 1 < (x := side_effect()) < 3 | |
| """) | |
| def test_method_call_receiver_evaluated_once(self) -> None: | |
| assert_single_evaluation(""" | |
| def check(): | |
| class C: | |
| def compute(self): | |
| return 1 | |
| def factory(): | |
| counter[0] += 1 | |
| return C() | |
| assert factory().compute() == 1 | |
| """) |
There was a problem hiding this comment.
Pull request overview
This PR extends pytest’s assertion rewriting to improve introspection for additional AST expression forms (subscript and ternary if-expressions) and to simplify method-call explanations, while adding a new “systematic coverage” test module to exercise many rewriting/introspection cases.
Changes:
- Add
visit_Subscriptto show container and key separately in assertion failure introspection. - Add
visit_IfExpto introspect ternary conditions while preserving short-circuit evaluation of branches. - Flatten method-call explanations so failures display
obj.method(...)directly (instead of a separate bound-method intermediate), plus add a large new coverage-oriented test suite.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
testing/test_assertrewrite_coverage.py |
New structured helper framework + a broad matrix of assertion-rewrite coverage tests (incl. semantics/single-eval checks). |
src/_pytest/assertion/rewrite.py |
Adds visitors for Subscript and IfExp, and changes Call handling to flatten method-call explanations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1064
to
+1086
| # Visit the object (receiver) for introspection. | ||
| obj_res, obj_expl = self.visit(attr.value) | ||
|
|
||
| # Visit arguments. | ||
| arg_expls = [] | ||
| new_args = [] | ||
| new_kwargs = [] | ||
| for arg in call.args: | ||
| res, expl = self.visit(arg) | ||
| arg_expls.append(expl) | ||
| new_args.append(res) | ||
| for keyword in call.keywords: | ||
| res, expl = self.visit(keyword.value) | ||
| new_kwargs.append(ast.keyword(keyword.arg, res)) | ||
| if keyword.arg: | ||
| arg_expls.append(keyword.arg + "=" + expl) | ||
| else: | ||
| arg_expls.append("**" + expl) | ||
|
|
||
| # Build the call using the rewritten object's attribute. | ||
| new_func = ast.Attribute(obj_res, attr.attr, ast.Load()) | ||
| new_call = ast.copy_location(ast.Call(new_func, new_args, new_kwargs), call) | ||
| res = self.assign(new_call) |
Comment on lines
+188
to
+207
| # Run without rewriting | ||
| plain_code = compile(src, "<test-plain>", "exec") | ||
| plain_ns: dict[str, object] = {} | ||
| if extra_ns is not None: | ||
| plain_ns.update(extra_ns) | ||
| exec(plain_code, plain_ns) | ||
| plain_func = cast(Callable[[], None], plain_ns["check"]) | ||
| plain_raised = False | ||
| try: | ||
| plain_func() | ||
| except AssertionError: | ||
| plain_raised = True | ||
|
|
||
| # Run with rewriting | ||
| mod = _rewrite_source(src) | ||
| rewritten_code = compile(mod, "<test-rewritten>", "exec") | ||
| rewritten_ns: dict[str, object] = {} | ||
| if extra_ns is not None: | ||
| rewritten_ns.update(extra_ns) | ||
| exec(rewritten_code, rewritten_ns) |
Update raises_group.py::test_assert_matches to expect the new flat method call format (where False = RaisesExc(TypeError).matches(...)) instead of the old nested format with a separate bound-method line. Also address Copilot review: use copy.deepcopy in assert_semantically_equivalent to isolate mutable state between the plain and rewritten execution runs. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Note: This is an AI-assisted experiment (Cursor + Claude Opus 4) that executed a plan after assessing initial details about assertion rewriting blind spots. More work is needed to make this clean and production-ready — this PR has known weaknesses and is meant as a starting point for discussion, not a final implementation.
testing/test_assertrewrite_coverage.py)visit_Subscriptforcontainer[key]introspection (shows container and key separately in failure messages)visit_IfExpfor ternary expression introspection (shows condition value while preserving short-circuit semantics)where result = obj.method(args)instead of nesting the bound-method intermediateBefore/After
Subscript (new)
IfExp (new)
Method calls (improved)
Known weaknesses / areas for improvement
visit_IfExponly introspects the condition, not the branch values (to preserve short-circuit) — a more sophisticated implementation could emit an if/else block to get branch values too<function ...>repr instead of variable name (xfail)Test plan
testing/test_assertrewrite_coverage.py(61 pass + 14 edge cases)testing/test_assertrewrite.pytests pass (no regressions)testing/test_assertion.pytests pass (no regressions)Made with Cursor
Summary by Sourcery
Improve assertion rewriting coverage and introspection, and add systematic tests to guard semantics and single-evaluation guarantees.
New Features:
Enhancements:
Tests: