Repository: pytest-dev/pytest
Pull request: Fix different behavior with unittest when warlus operator
The `AssertionRewriter` class did not handle the assertions with the `ast.NamedExpr` (aka 'walrus operator'), that changed the value of a variable used in the assertion (an example can be seen in the related issue or in the tests added). 

This caused different behavior from unittest/python and pytest.

This PR aims to handle this case (although very remote)

_Addresses python version >= 3.8_

closes #10743

<!--
Thanks for submitting a PR, your contribution is really appreciated!

Here is a quick checklist that should be present in PRs.

- [ ] Include documentation when adding new features.
- [ ] Include new tests or update existing tests when applicable.
- [X] Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

- [ ] Add text like ``closes #XYZW`` to the PR description and/or commits (where ``XYZW`` is the issue number). See the [github docs](https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for more information.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

- [ ] Create a new changelog file in the `changelog` folder, with a name like `<ISSUE NUMBER>.<TYPE>.rst`. See [changelog/README.rst](https://github.com/pytest-dev/pytest/blob/main/changelog/README.rst) for details.

  Write sentences in the **past or present tense**, examples:

  * *Improved verbose diff output with sequences.*
  * *Terminal summary statistics now use multiple colors.*

  Also make sure to end the sentence with a `.`.

- [ ] Add yourself to `AUTHORS` in alphabetical order.
-->


Problem statement:
Walrus operator causes different behavior in PyTest.
I am currently testing the following function but find that the function passes the test in the normal Python terminal but fails in a PyTest run. The walrus operator is relatively new and not many people use it. I think that there may be an inconsistency in the execution environment.

```
import numpy as np

def test_walrus_conversion():
    a = np.random.random(16)
    assert not np.array_equal(a, a := a.astype(np.uint8))
    assert np.all(a == 0)
```

Hints from issue discussion:
It's very likely a missed case in the asset rewrite


Hi, I did some tests on this since I was curious about how this works.
I created a module like this 

```
def my_func(before, after):
    return before == after

def change_value(value):
    return value.lower()

def test_walrus_conversion():
    a = "Hello"
    assert not my_func(a, a := change_value(a))
    assert a == "hello"
```
and run it. It fails as the issuer stated. Then I added """PYTEST_DONT_REWRITE""" at the beginning of the module and now it passes. 
If it's ok with you I'd like to try to fix this.

As a side note, this is a very edge case and I don't know if it is clean to write something like this. Anyway, since the unittest mode says that this test should pass, we should align with that



@aless10 thnks for providing a self-contained reproducer

i wonder if it also behaves that way if change_value is inlined

we should align with whatever is the result when using `--assert=plain`
Quick update: I think I fixed the bug by telling the AssertionRewrite to visit the NamedExpr expr and not by using the generic visitor.
That said, I got into another possible problem, because right now the error messages are not what we expect. Here is an example

````
def test_walrus_conversion_fails():
    a = "Hello"
    assert a == (a := a.lower())
````
this test fails correctly, but this is the message I've got

````
    def test_walrus_conversion_fails():
        a = "Hello"
>       assert a == (a := a.lower())
E       AssertionError: assert 'hello' == 'hello'
E           hello

````

and this is not what we expect, it's even worse! I'm trying to fix this behavior and I succeeded, but then this other test


````
def test_walrus_conversion_fails():
    a = "Hello"
    assert a != (a := a.lower())
    assert a == 'hello'

````
fails with this message
```
>       assert a == 'hello'
E       AssertionError: assert 'Hello' == 'hello'
E         - hello
E         ? ^
E         + Hello
E         ? ^

```
, because the solution I found does not remember that we change the variable value. 
Another bit on this topic. It looks like there is something strange on how this thing works with mutable objects. For example

```
def change_me(d: dict) -> dict:
    d['change'] = True
    return d
        
def my_func(before, after):
    return before == after
            
def test_change_me():
    a = {}
    assert my_func(a, change_me(a))
    assert a == {"change": True}
```
this test has the same behavior in pytest and unittest, but it looks like the same pattern. In fact, there is a variable that is passed to a function with another argument that changes its value. 
To me these two tests below

`````
def change_me(d: dict) -> dict:
    d['change'] = True
    return d

def change_me_str(value: str) -> str:
    return value.lower()
        
def my_func(before, after):
    return before == after

def test_walrus_conversion():
    a = "Hello"
    assert not my_func(a, a := change_me_str(a))
    assert a == "hello"


def test_change_me():
    a = {}
    assert my_func(a, change_me(a))
    assert a == {"change": True}
````

should behave the same, but in the first case (with the walrus operator) the change is not applied until the function is executed. 
Ok that strings are immutable and dict aren't but it's very strange to understand and explain. 

Aliasing is important to consider, assertion side effects can hide deep changes, which is acceptable (just normal python)

We might have to evaluate when to get a repr

We better warn that we only capture references, not deep mutable state and strongly recommend doing assertion on immutable without side effects

Commit message: add version check for py<38

Patch to review:
diff --git a/AUTHORS b/AUTHORS
index 0395feceb60..448b71d3a8a 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -12,6 +12,7 @@ Adam Uhlir
 Ahn Ki-Wook
 Akiomi Kamakura
 Alan Velasco
+Alessio Izzo
 Alexander Johnson
 Alexander King
 Alexei Kozlenok
diff --git a/changelog/10743.bugfix.rst b/changelog/10743.bugfix.rst
new file mode 100644
index 00000000000..db8eb73b072
--- /dev/null
+++ b/changelog/10743.bugfix.rst
@@ -0,0 +1 @@
+Fixed different behavior from std lib unittest of asserts with expression that contains the walrus operator in it that changes the value of a variable.
diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py
index 42664add432..2f011bba824 100644
--- a/src/_pytest/assertion/rewrite.py
+++ b/src/_pytest/assertion/rewrite.py
@@ -44,6 +44,11 @@
 if TYPE_CHECKING:
     from _pytest.assertion import AssertionState
 
+if sys.version_info > (3, 8):
+    namedExpr = ast.NamedExpr
+else:
+    namedExpr = ast.Expr
+
 
 assertstate_key = StashKey["AssertionState"]()
 
@@ -666,7 +671,7 @@ def run(self, mod: ast.Module) -> None:
         if doc is not None and self.is_rewrite_disabled(doc):
             return
         pos = 0
-        lineno = 1
+        item = None
         for item in mod.body:
             if (
                 expect_docstring
@@ -937,6 +942,17 @@ def visit_Assert(self, assert_: ast.Assert) -> List[ast.stmt]:
                 ast.copy_location(node, assert_)
         return self.statements
 
+    def visit_NamedExpr(self, name: namedExpr) -> Tuple[namedExpr, str]:
+        # Display the repr of the target name if it's a local variable or
+        # _should_repr_global_name() thinks it's acceptable.
+        locs = ast.Call(self.builtin("locals"), [], [])
+        target_id = name.target.id  # type: ignore[attr-defined]
+        inlocs = ast.Compare(ast.Str(target_id), [ast.In()], [locs])
+        dorepr = self.helper("_should_repr_global_name", name)
+        test = ast.BoolOp(ast.Or(), [inlocs, dorepr])
+        expr = ast.IfExp(test, self.display(name), ast.Str(target_id))
+        return name, self.explanation_param(expr)
+
     def visit_Name(self, name: ast.Name) -> Tuple[ast.Name, str]:
         # Display the repr of the name if it's a local variable or
         # _should_repr_global_name() thinks it's acceptable.
@@ -1072,6 +1088,7 @@ def visit_Compare(self, comp: ast.Compare) -> Tuple[ast.expr, str]:
             res: ast.expr = ast.BoolOp(ast.And(), load_names)
         else:
             res = load_names[0]
+
         return res, self.explanation_param(self.pop_format_context(expl_call))
 
 
diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py
index 3c98392ed98..624947d7416 100644
--- a/testing/test_assertrewrite.py
+++ b/testing/test_assertrewrite.py
@@ -1265,6 +1265,163 @@ def test_simple_failure():
         result.stdout.fnmatch_lines(["*E*assert (1 + 1) == 3"])
 
 
+@pytest.mark.skipif(
+    sys.version_info < (3, 8), reason="walrus operator not available in py<38"
+)
+class TestIssue10743:
+    def test_assertion_walrus_operator(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def my_func(before, after):
+                return before == after
+
+            def change_value(value):
+                return value.lower()
+
+            def test_walrus_conversion():
+                a = "Hello"
+                assert not my_func(a, a := change_value(a))
+                assert a == "hello"
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_operator_dont_rewrite(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            'PYTEST_DONT_REWRITE'
+            def my_func(before, after):
+                return before == after
+
+            def change_value(value):
+                return value.lower()
+
+            def test_walrus_conversion_dont_rewrite():
+                a = "Hello"
+                assert not my_func(a, a := change_value(a))
+                assert a == "hello"
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_inline_walrus_operator(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def my_func(before, after):
+                return before == after
+
+            def test_walrus_conversion_inline():
+                a = "Hello"
+                assert not my_func(a, a := a.lower())
+                assert a == "hello"
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_inline_walrus_operator_reverse(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def my_func(before, after):
+                return before == after
+
+            def test_walrus_conversion_reverse():
+                a = "Hello"
+                assert my_func(a := a.lower(), a)
+                assert a == 'hello'
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_no_variable_name_conflict(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_conversion_no_conflict():
+                a = "Hello"
+                assert a == (b := a.lower())
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 1
+        result.stdout.fnmatch_lines(["*AssertionError: assert 'Hello' == 'hello'"])
+
+    def test_assertion_walrus_operator_true_assertion_and_changes_variable_value(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_conversion_succeed():
+                a = "Hello"
+                assert a != (a := a.lower())
+                assert a == 'hello'
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_operator_fail_assertion(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_conversion_fails():
+                a = "Hello"
+                assert a == (a := a.lower())
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 1
+        # This is not optimal as error message but it depends on how the rewrite is structured
+        result.stdout.fnmatch_lines(["*AssertionError: assert 'hello' == 'hello'"])
+
+    def test_assertion_walrus_operator_boolean_composite(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_operator_change_boolean_value():
+                a = True
+                assert a and True and ((a := False) is False) and (a is False) and ((a := None) is None)
+
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_operator_compare_boolean_fails(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_operator_change_boolean_value():
+                a = True
+                assert not (a and ((a := False) is False))
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 1
+        # This is not optimal as error message but it depends on how the rewrite is structured
+        result.stdout.fnmatch_lines(["*assert not (False)"])
+
+    def test_assertion_walrus_operator_boolean_none_fails(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_operator_change_boolean_value():
+                a = True
+                assert not (a and ((a := None) is None))
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 1
+        # This is not optimal as error message but it depends on how the rewrite is structured
+        result.stdout.fnmatch_lines(["*assert not (None)"])
+
+
 @pytest.mark.skipif(
     sys.maxsize <= (2**31 - 1), reason="Causes OverflowError on 32bit systems"
 )


Code review comments:
diff_hunk: @@ -1265,6 +1265,163 @@ def test_simple_failure():
         result.stdout.fnmatch_lines(["*E*assert (1 + 1) == 3"])
 
 
+@pytest.mark.skipif(
+    sys.version_info < (3, 8), reason="walrus operator not available in py<38"
+)
+class TestIssue10743:
+    def test_assertion_walrus_operator(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def my_func(before, after):
+                return before == after
+
+            def change_value(value):
+                return value.lower()
+
+            def test_walrus_conversion():
+                a = "Hello"
+                assert not my_func(a, a := change_value(a))
+                assert a == "hello"
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_operator_dont_rewrite(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            'PYTEST_DONT_REWRITE'
+            def my_func(before, after):
+                return before == after
+
+            def change_value(value):
+                return value.lower()
+
+            def test_walrus_conversion_dont_rewrite():
+                a = "Hello"
+                assert not my_func(a, a := change_value(a))
+                assert a == "hello"
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_inline_walrus_operator(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def my_func(before, after):
+                return before == after
+
+            def test_walrus_conversion_inline():
+                a = "Hello"
+                assert not my_func(a, a := a.lower())
+                assert a == "hello"
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_inline_walrus_operator_reverse(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def my_func(before, after):
+                return before == after
+
+            def test_walrus_conversion_reverse():
+                a = "Hello"
+                assert my_func(a := a.lower(), a)
+                assert a == 'hello'
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_no_variable_name_conflict(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_conversion_no_conflict():
+                a = "Hello"
+                assert a == (b := a.lower())
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 1
+        result.stdout.fnmatch_lines(["*AssertionError: assert 'Hello' == 'hello'"])
+
+    def test_assertion_walrus_operator_true_assertion_and_changes_variable_value(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_conversion_succeed():
+                a = "Hello"
+                assert a != (a := a.lower())
+                assert a == 'hello'
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
+    def test_assertion_walrus_operator_fail_assertion(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            def test_walrus_conversion_fails():
+                a = "Hello"
+                assert a == (a := a.lower())
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 1
+        # This is not optimal as error message but it depends on how the rewrite is structured
path: testing/test_assertrewrite.py
text: @user1:
I noticed this as well, can't we save the first `a` value in a separate temporary variable, before evaluating the walrus operator? This would provide the same (correct) error message as in `test_assertion_walrus_no_variable_name_conflict`.

@author:
I tried to do so, but with my approach I did not change the value of the variable `a` but I saved the new value of `a` in another temporary variable. The problem with this is that if that variable is used later in another assert there will be an error. I think is doable, I just need to try different approach. Thanks 

@user1:
>  The problem with this is that if that variable is used later in another assert there will be an error

IIUC the problem, you can use `self.variable()` to obtain a new, unique variable to save the value to. This would prevent any conflicts in later asserts.

@author:
Thanks for the tip: I refactored using the `variable` method but I don't think this is enough to solve the problem. For example in this test case
```
def test_walrus_conversion_succeed():
    a = "Hello"
    assert a != (a := a.lower())
    assert a == 'hello'
```
if I change the name of the first "a", then I got an error because it is referencing something that has no reference (in fact "a" is defined outside the first assert statement. I tried to change the name of the new "a" in the walrus operator, but then the second assert fails because the value of "a" is still the original one. 
The only solution that came to my mind is using a map/state that lives through the asserts that enables me to retrieves the values if they have changed.
That said, this situation is very rare and the tests cases that came to my mind are far from being used in real case scenario (but this is only my opinion). I'm thinking if the juice worth the squeeze

@user1:
> That said, this situation is very rare and the tests cases that came to my mind are far from being used in real case scenario (but this is only my opinion). I'm thinking if the juice worth the squeeze

Yeah if it proves too problematic, I'm happy with having just the original fix, and we can instead add a note to the rewrite docs somewhere about this corner case. I agree it should be very rare to happen in practice.

Please feel free to revert to the previous working version. 

@author:
So, after https://github.com/pytest-dev/pytest/pull/10788, all the tests pass. Right now, we have covered all of the cases that are tested in `test_assertrewrite.py::TestIssue10743` where I tried to imagine most of the corner cases. Let me know if you like my solution or if you prefer to revert and add a note for this remote case. Thanks