Skip to content
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

str() on the pytest.raises context variable doesn't behave same as normal exception catch #5412

Closed
fiendish opened this issue Jun 6, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@fiendish
Copy link

commented Jun 6, 2019

Pytest 4.6.2, macOS 10.14.5

try:
    raise LookupError(
        f"A\n"
        f"B\n"
        f"C"
    )
except LookupError as e:
    print(str(e))

prints

A
B
C

But

with pytest.raises(LookupError) as e:
    raise LookupError(
        f"A\n"
        f"B\n"
        f"C"
    )

print(str(e))

prints

:3: LookupError: A

In order to get the full error message, one must do str(e.value), which is documented, but this is a different interaction. Any chance the behavior could be changed to eliminate this gotcha?


Pip list gives

Package            Version  Location
------------------ -------- ------------------------------------------------------
apipkg             1.5
asn1crypto         0.24.0
atomicwrites       1.3.0
attrs              19.1.0
aws-xray-sdk       0.95
boto               2.49.0
boto3              1.9.51
botocore           1.12.144
certifi            2019.3.9
cffi               1.12.3
chardet            3.0.4
Click              7.0
codacy-coverage    1.3.11
colorama           0.4.1
coverage           4.5.3
cryptography       2.6.1
decorator          4.4.0
docker             3.7.2
docker-pycreds     0.4.0
docutils           0.14
ecdsa              0.13.2
execnet            1.6.0
future             0.17.1
idna               2.8
importlib-metadata 0.17
ipaddress          1.0.22
Jinja2             2.10.1
jmespath           0.9.4
jsondiff           1.1.1
jsonpickle         1.1
jsonschema         2.6.0
MarkupSafe         1.1.1
mock               3.0.4
more-itertools     7.0.0
moto               1.3.7
neobolt            1.7.10
neotime            1.7.4
networkx           2.1
numpy              1.15.0
packaging          19.0
pandas             0.24.2
pip                19.1.1
pluggy             0.12.0
prompt-toolkit     2.0.9
py                 1.8.0
py2neo             4.2.0
pyaml              19.4.1
pycodestyle        2.5.0
pycparser          2.19
pycryptodome       3.8.1
Pygments           2.3.1
pyOpenSSL          19.0.0
pyparsing          2.4.0
pytest             4.6.2
pytest-cache       1.0
pytest-codestyle   1.4.0
pytest-cov         2.6.1
pytest-forked      1.0.2
python-dateutil    2.7.3
python-jose        2.0.2
pytz               2018.5
PyYAML             5.1
requests           2.21.0
requests-mock      1.5.2
responses          0.10.6
s3transfer         0.1.13
setuptools         41.0.1
six                1.11.0
sqlite3worker      1.1.7
tabulate           0.8.3
urllib3            1.24.3
wcwidth            0.1.7
websocket-client   0.56.0
Werkzeug           0.15.2
wheel              0.33.1
wrapt              1.11.1
xlrd               1.1.0
xmltodict          0.12.0
zipp               0.5.1
@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Any chance the behavior could be changed to eliminate this gotcha?

What do you suggest?

Proxying through to the exceptions __str__?

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Hi @fiendish,

Indeed this is a bit confusing.

Currently ExceptionInfo objects (which is pytest.raises returns to the context manager) implements __str__ like this:

def __str__(self):
if self._excinfo is None:
return repr(self)
entry = self.traceback[-1]
loc = ReprFileLocation(entry.path, entry.lineno + 1, self.exconly())
return str(loc)

I don't see much use for this, I would rather it didn't implement __str__ at all and let __repr__ take over, which would show something like:

<ExceptionInfo LookupError tb=10>

Which makes it more obvious that this is not what the user intended with str(e) probably.

So I think a good solution is to simply delete the __str__ method.

Thoughts?

Also, @fiendish which Python version are you using?

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

So I think a good solution is to simply delete the __str__ method.

Makes sense to me.

@fiendish

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Python 3.7.3

My ideal outcome would be for str(e) to act the same as str(e.value), but I can understand if that isn't desired.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

My ideal outcome would be for str(e) to act the same as str(e.value), but I can understand if that isn't desired.

I understand, but I think it is better to be explicit here, because users might use print(e) to see what e is, assume it is the exception value, and then get confused later when it actually isn't (an isinstance check or accessing e.args).

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

+1 for deleting the current __str__ implementation
-1 for proxying it to the underlying e.value

the ExceptionInfo object is not the exception and anything that makes it look more like the exception is just going to add to the confusion

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 6, 2019

@sk1p sk1p referenced this issue Jul 1, 2019

Merged

Fluctuation EM GUI #385

sk1p added a commit to LiberTEM/LiberTEM that referenced this issue Jul 1, 2019

Updates for new pytest release
str() for ExceptionInfo changed its format, so we are now using
the newly-introduced `ExceptionInfo.match(regexp)` method instead.
Depend on pytest>5 but <6 to prevent this kind of surprise in the future.

See also: pytest-dev/pytest#5412

rohanpm added a commit to rohanpm/pubtools-pulplib that referenced this issue Jul 1, 2019

Fix test failures with pytest 5
str() behavior on pytest ExceptionInfo objects was changed in
pytest 5.0.0: pytest-dev/pytest#5412

This caused various tests to fail, as it was assumed that str()
on those objects would include exception messages. Update the
tests to check the exceptions instead, rather than the
ExceptionInfo wrappers.

danielballan added a commit to danielballan/bluesky that referenced this issue Jul 2, 2019

TST: Fix our usage of pytest ExceptionInfo.
One test has started failing consistently on the OSX builds, and the
cause is a pytest major release that dropped on June 28. Travis' Ubuntu
virtual machines come with an older version of pytest included, and we do not
force it to upgrade, so we have only encountered this on the OSX builds
so far.

The usage

```python
with pytest.raises(Exception) as exc
```

puts an [``ExceptionInfo``](https://docs.pytest.org/en/latest/reference.html#exceptioninfo)
object into ``exc``, not the exception itself. We were trying ``exc``
like an exception object and relying on the fact that
``ExceptionInfo.__str__`` returned the same thing as ``exc.__str__``
would. The
[pytest 5.0.0 release](https://docs.pytest.org/en/latest/changelog.html#pytest-5-0-0-2019-06-28)
broke that assumption. See pytest-dev/pytest#5412.

@chisholm chisholm referenced this issue Jul 3, 2019

Merged

Stix21master #275

MartinBasti added a commit to MartinBasti/operators-manifests-push-service that referenced this issue Jul 3, 2019

Pytest 5.0.0 fixes
because of this change pytest-dev/pytest#5412

Signed-off-by: Martin Bašti <mbasti@redhat.com>

shashank88 added a commit to shashank88/arctic that referenced this issue Jul 5, 2019

Fix tests broken due to pytest-dev/pytest#5412
This breaks our tests as we use str(e) everywhere and as the above
PR changes the behaviour of str to be the same as repr which is top
print the Exception info Object, it breaks our tests.

shashank88 added a commit to shashank88/arctic that referenced this issue Jul 5, 2019

Fix tests broken due to pytest-dev/pytest#5412
This breaks our tests as we use str(e) everywhere and as the above
PR changes the behaviour of str to be the same as repr which is top
print the Exception info Object, it breaks our tests.

shashank88 added a commit to manahl/arctic that referenced this issue Jul 5, 2019

MartinBasti added a commit to release-engineering/operators-manifests-push-service that referenced this issue Jul 11, 2019

Pytest 5.0.0 fixes
because of this change pytest-dev/pytest#5412

Signed-off-by: Martin Bašti <mbasti@redhat.com>

MartinBasti added a commit to MartinBasti/gunicorn that referenced this issue Jul 11, 2019

Fix pytest 5.0.0 compatibility
pytest.raises() returns exception info not the exception itself. They
changed implementation of exception info, so now .value property must be
used to get the exception instance and have proper output from str()
method.

pytest-dev/pytest#5412

Signed-off-by: Martin Bašti <mbasti@redhat.com>

berkerpeksag added a commit to benoitc/gunicorn that referenced this issue Jul 17, 2019

Fix pytest 5.0.0 compatibility
pytest.raises() returns exception info not the exception itself. They
changed implementation of exception info, so now .value property must be
used to get the exception instance and have proper output from str()
method.

pytest-dev/pytest#5412

Signed-off-by: Martin Bašti <mbasti@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.