Repository: pytest-dev/pytest
Pull request: Improve reporting from __iter__ exceptions
Fixes #7966

Previously, the assertion rewrite mechanism would fail to capture raised errors within ``__iter__`` and suggest the error is within ``__repr__``, which this new reporting now fixes.

<!--
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.
- [X] 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:
Failure when trying to call __repr__ because __iter__ is called?
## 1. Problem

The following code 

```python
class A:
    def __iter__(self):
        raise ValueError()

    def __eq__(self, o: object) -> bool:
        return self is o


def test_a():
    assert A() == A()
```

...will throw:

```
    def test_a():
>       assert A() == A()
E       assert <tests.test_p...0019C132F1490> == <tests.test_p...0019C132F12B0>
E         (pytest_assertion plugin: representation of details failed: XXX\test_pp.py:3: ValueError.
E          Probably an object has a faulty __repr__.)
```

...because `__iter__` is called in order to `repr()` the object?

Is there a way to avoid `__iter__` being called when a test fails?

## 2. Setup

#### Python 3.8.2 | pytest 6.1.1

Hints from issue discussion:
part of the assertion representation logic does try various things to tell more about the object

the isiterable helper unfortunately calls `__iter__` instead of just verifying its existence

it only checks for type-error, which doesn't account for broken implementations of iter
we should decide whether we want to uphold the invariant that iter calls should only raise type errors, or whether we need to do something better

in any case the error needs to be better
Thanks, I actually did not think that there was some conventions about the type of exceptions being raised by special methods... 

I'm working on a custom interpreter for a different language, so I simply had a different `MyTypeError`. Making a kind of hybrid error inheriting both type errors actually fix the issue.
CC @nicoddemus after reviewing i beleive we shoudl add 2 additional failure modes

a) types that wrap data but dont proviede `__iter__` should diff a mu
ltiline repr
b) types that fail `__iter__` should be considered differently

```

In [1]: 
   ...: 
   ...: class A:
   ...:     def __iter__(self):
   ...:         raise ValueError()
   ...: 
   ...:     def __eq__(self, o: object) -> bool:
   ...:         return self is o
   ...: 

In [2]: 

In [2]: a = A()

In [9]: pprint.pformat(a)
Out[9]: '<__main__.A object at 0x7fab6fda9d30>'

In [10]: from _pytest.assertion.util import _pformat_dispatch

In [11]: _pformat_dispatch(a)
Out[11]: '<__main__.A object at 0x7fab6fda9d30>'

In [12]: 
```

i believe a better error should be triggered

 
I agree with b), however I don't understand what you mean with a)... why use a multiline repr for types that _don't_ have a `__iter__` method?
@nicoddemus i have some helper pbjects around that just wrap around other sutff

also openapi types dont necessarily iterate but have a nested multiline repr
Hmm wait, I might be missing something, what do you mean by "multiline repr"? 🤔 
@nicoddemus any type where repr is either multiline on its own or by using something some `pprint.pformat`


You mean calling `__repr__().split()` and see if there are more than one line, and treat that accordingly? I see.
Just use diff on it

Commit message: improve error msg and test

Patch to review:
diff --git a/changelog/7966.bugfix.rst b/changelog/7966.bugfix.rst
new file mode 100644
index 00000000000..40cad46c69d
--- /dev/null
+++ b/changelog/7966.bugfix.rst
@@ -0,0 +1 @@
+Assertion rewrite mechanism now gives a seperate, more detailed error message from failures within __iter__.
diff --git a/src/_pytest/assertion/util.py b/src/_pytest/assertion/util.py
index fc5dfdbd5ba..d20c2363eb3 100644
--- a/src/_pytest/assertion/util.py
+++ b/src/_pytest/assertion/util.py
@@ -134,6 +134,13 @@ def isiterable(obj: Any) -> bool:
         return not istext(obj)
     except TypeError:
         return False
+    except Exception as e:
+        raise ValueError(
+            [
+                f"pytest_assertion plugin: unexpected exception {e!r} while testing object {obj!r}",
+                ", probably from __iter__",
+            ]
+        )
 
 
 def has_default_eq(
@@ -195,13 +202,20 @@ def assertrepr_compare(
                 explanation = _notin_text(left, right, verbose)
     except outcomes.Exit:
         raise
-    except Exception:
-        explanation = [
-            "(pytest_assertion plugin: representation of details failed: {}.".format(
-                _pytest._code.ExceptionInfo.from_current()._getreprcrash()
-            ),
-            " Probably an object has a faulty __repr__.)",
-        ]
+    except Exception as e:
+        if (
+            isinstance(e, ValueError)
+            and (len(e.args) == 1)
+            and ("__iter__" in str(e.args[0]))
+        ):
+            explanation = e.args[0]
+        else:
+            explanation = [
+                "(pytest_assertion plugin: representation of details failed: {}.".format(
+                    _pytest._code.ExceptionInfo.from_current()._getreprcrash()
+                ),
+                " Probably an object has a faulty __repr__.)",
+            ]
 
     if not explanation:
         return None
diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py
index e1b71ded69c..62c6bb8d4bc 100644
--- a/testing/test_assertrewrite.py
+++ b/testing/test_assertrewrite.py
@@ -685,6 +685,21 @@ def myany(x) -> bool:
         assert msg is not None
         assert "<MY42 object> < 0" in msg
 
+    def test_assert_handling_raise_in__iter__(self) -> None:
+        def f() -> None:
+            class A:
+                def __iter__(self):
+                    raise ValueError()
+
+                def __eq__(self, o: object) -> bool:
+                    return self is o
+
+            assert A() == A()
+
+        msg = getmsg(f)
+        assert msg is not None
+        assert "__iter__" in msg and "__repr__" not in msg
+
     def test_formatchar(self) -> None:
         def f() -> None:
             assert "%test" == "test"  # type: ignore[comparison-overlap]


Code review comments:
diff_hunk: @@ -134,6 +134,13 @@ def isiterable(obj: Any) -> bool:
         return not istext(obj)
     except TypeError:
         return False
+    except Exception as e:
+        raise ValueError(
path: src/_pytest/assertion/util.py
text: @user1:
I think we should probably just replace `except TypeError:` with `except Exception:` here - if calling `iter(obj)` raises an error, it seems reasonable to treat that as "not iterable" rather than a special error case.

@author:
Hm I think the error message is helpful if people don't expect it, but I can see that this adds to things to maintain