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

No longer can extract message from exceptions in 5+ versions #5579

Closed
cscetbon opened this issue Jul 8, 2019 · 15 comments
Closed

No longer can extract message from exceptions in 5+ versions #5579

cscetbon opened this issue Jul 8, 2019 · 15 comments
Labels
type: question general question, might be closed after 2 weeks of inactivity

Comments

@cscetbon
Copy link

cscetbon commented Jul 8, 2019

I can't extract the message of an exception using str(my_exception) when using pytest 5+. Here is an example :

E       AssertionError: assert 'blabla' in '<ExceptionInfo Exception tblen=1>'
E        +  where '<ExceptionInfo Exception tblen=1>' = str(<ExceptionInfo Exception tblen=1>)

That test succeeds in previous versions (4.6.4 for instance)

I've been able to test it using the following example :

import pytest

def test_ex():
    with pytest.raises(Exception) as e:
        raise Exception('blabla')

    assert 'blabla' in str(e)
@banderson5586
Copy link

assert 'blabla' in str(e.value)

Haven't looked at release notes yet to see why but accessing the exception text changed slightly.

@cscetbon
Copy link
Author

cscetbon commented Jul 8, 2019

@banderson5586 but why accessing the error message using pytest is different from the way it's done in python 2/3 ? it does not make any sense to me.

@nicoddemus
Copy link
Member

Hi,

Here's the related CHANGELOG entry:

#5412: ExceptionInfo objects (returned by pytest.raises) now have the same str representation as repr, which avoids some confusion when users use print(e) to inspect the object.

So it was mostly to avoid confusion. Note that pytest.raises returns a ExceptionInfo object when used in a with context, not the exception object itself.

Here are the docs for ExceptionInfo: https://docs.pytest.org/en/latest/reference.html#exceptioninfo

As can be seen in the docs, ExceptionInfo objects contain a lot of other attributes, so what happened before is that it was a little confusing when someone printed an ExceptionInfo object and it behave as if it was the exception value.

@nicoddemus nicoddemus added the type: question general question, might be closed after 2 weeks of inactivity label Jul 8, 2019
@cscetbon
Copy link
Author

cscetbon commented Jul 9, 2019

Understood. This change is probably gonna break a ton of unit tests ...

@nicoddemus
Copy link
Member

Let's see, we are hopíng not, as this was not really documented behavior.

I'm closing this for now, feel free to follow up with further questions though.

demartinofra added a commit to demartinofra/aws-parallelcluster that referenced this issue Jul 15, 2019
related to pytest-dev/pytest#5579

Signed-off-by: Francesco De Martino <fdm@amazon.com>
demartinofra added a commit to demartinofra/aws-parallelcluster that referenced this issue Jul 15, 2019
related to pytest-dev/pytest#5579

Signed-off-by: Francesco De Martino <fdm@amazon.com>
demartinofra added a commit to demartinofra/aws-parallelcluster that referenced this issue Jul 15, 2019
related to pytest-dev/pytest#5579

Signed-off-by: Francesco De Martino <fdm@amazon.com>
demartinofra added a commit to aws/aws-parallelcluster that referenced this issue Jul 15, 2019
related to pytest-dev/pytest#5579

Signed-off-by: Francesco De Martino <fdm@amazon.com>
@stadelmanma
Copy link

@nicoddemus I understand the change but it did smash a good bit of my test suite out of the blue when my CI suite pulled v5 instead of v4.X. I didn't realize I was using undocumented behavior which in the end is on me but I suspect several others will get hit in the face by this as well.

Maybe adding an extra note somewhere that statements such as assert "string" in str(err) will no longer work would help with the pain. I read the CHANGELOG prior to finding this issue but I didn't realize the impact of #5412 until seeing it detailed out here.

@cscetbon
Copy link
Author

I don't know @stadelmanma how much time you spent in order to find this issue but I still think it would make more sense to support the old behavior and avoid breaking uncountable tests.

@nicoddemus
Copy link
Member

Maybe adding an extra note somewhere that statements such as assert "string" in str(err) will no longer work would help with the pain

Sorry this caused problems, we usually try to avoid that as much as possible.

I would be glad to review/merge a PR with improved docs and/or to the README! 👍

@blueyed
Copy link
Contributor

blueyed commented Jul 22, 2019

(Just for information / as a data point: I've also seen this rather often in test suites (but not my own ;)) upgrading to pytest 5 by now. But usually without much effort (AFAICS).)

@wjwwood
Copy link
Contributor

wjwwood commented Jul 22, 2019

Broke quite a few of our tests too, I agree with @stadelmanma's comment (#5579 (comment)). I didn't connect the CHANGELOG content to this consequence either.

Edit: README -> CHANGELOG

wjwwood added a commit to ros2/launch that referenced this issue Jul 22, 2019
See: pytest-dev/pytest#5579

Signed-off-by: William Woodall <william@osrfoundation.org>
@stadelmanma
Copy link

I agree the README would provide better visibility but I don't think this kind of notice fits well on it. The CHANGLOG is the more appropriate place and maybe somewhere in the general docs but I'll need to scan over those tomorrow to see if any pages make sense.

@cscetbon When I saw all my tests break I took the following steps:

  1. Check to see if there was a major version change in the pytest installed (check)
  2. Seeing a new major version, check the CHANGELOG for any obvious breaking changes (miss)
  3. Check if any relevant issues seem to describe the problem (check)

All in all it only took me about 5-10 minutes to figure out the problem since you already found it. I imagine most devs will probably take the same steps unless they've never been burned by a dependency changed. (Eventually Google SEO will probably funnel people here directly)

@nicoddemus, perhaps there are a few options to allow this behavior back:

  1. Allow this as deprecated behavior, disabled by default but enabled via config opt? This gives people time to migrate away from the pattern.
  2. Allow "in" to work like str(err) used to by having the error text as part of the output when str/repr is called.
  3. (still requires code refactors but pairs well with opt 1.) Allow "in" to work on an ExceptionInfo object as if we were doing str(err) in v4.x but we would do "foo" in err instead of "foo" in str(err).

@wjwwood
Copy link
Contributor

wjwwood commented Jul 23, 2019

Sorry, README was a typo, I meant CHANGELOG too.

@nicoddemus
Copy link
Member

Definitely meant CHANGELOG as well, not README, sorry about that.

@nicoddemus, perhaps there are a few options to allow this behavior back

Those are all sensible solutions, thanks, but I'm not sure adding a new option (and now having to maintain it until at least pytest 6.0) will actually help things. I believe it will probably only add to the confusion.

@cscetbon
Copy link
Author

@cscetbon
All in all it only took me about 5-10 minutes to figure out the problem since you already found it. I imagine most devs will probably take the same steps unless they've never been burned by a dependency changed. (Eventually Google SEO will probably funnel people here directly)

Neat. I guess you’re right, that issue helps understanding it faster. I’m not sure everybody will look into the dependencies but yeah Google will hopefully funnel them here.

@nicoddemus, perhaps there are a few options to allow this behavior back:

  1. Allow this as deprecated behavior, disabled by default but enabled via config opt? This gives people time to migrate away from the pattern.

Could be interesting and that’s usually what I expect from a breaking change ^. I wouldn’t vote for a config option though. It could be deprecated but supported for a few versions before being unsupported.

@nicoddemus
Copy link
Member

nicoddemus commented Jul 23, 2019

Could be interesting and that’s usually what I expect from a breaking change ^. I wouldn’t vote for a config option though. It could be deprecated but supported for a few versions before being unsupported.

Agree, in retrospect I think that would be the best course of action. At the time we didn't thought it would bring much breakage... 😞

I opened #5648 to at least make that more explicit on the changelog.

wjwwood added a commit to ros2/launch that referenced this issue Jul 24, 2019
See: pytest-dev/pytest#5579

Signed-off-by: William Woodall <william@osrfoundation.org>
wjwwood added a commit to ros2/launch that referenced this issue Jul 24, 2019
…ts to true"" (#277)

* Revert "Revert "[execute_process] emulate_tty configurable and defaults to true (#265)" (#276)"

This reverts commit 15af530.

* add option to strip ansi escape sequences from output in launch_testing

Signed-off-by: William Woodall <william@osrfoundation.org>

* move registration of event handlers before including test file

Signed-off-by: William Woodall <william@osrfoundation.org>

* replace \r\n with \n too, because you get this in emulated tty mode

Signed-off-by: William Woodall <william@osrfoundation.org>

* fix a new test failure due a change in how pytest represents exceptions

See: pytest-dev/pytest#5579

Signed-off-by: William Woodall <william@osrfoundation.org>

* refactor to not use YAML in eval of emulate_tty option

Signed-off-by: William Woodall <william@osrfoundation.org>

* fix typo

Signed-off-by: William Woodall <william@osrfoundation.org>

* refactor emulate_tty tests and test constructor argument

Signed-off-by: William Woodall <william@osrfoundation.org>

* change default for emulate_tty to be False and fix warnings

Signed-off-by: William Woodall <william@osrfoundation.org>
tomalrussell added a commit to nismod/smif that referenced this issue Jul 30, 2019
piraka9011 pushed a commit to aws-ros-dev/launch that referenced this issue Aug 16, 2019
…ts to true"" (ros2#277)

* Revert "Revert "[execute_process] emulate_tty configurable and defaults to true (ros2#265)" (ros2#276)"

This reverts commit 15af530.

* add option to strip ansi escape sequences from output in launch_testing

Signed-off-by: William Woodall <william@osrfoundation.org>

* move registration of event handlers before including test file

Signed-off-by: William Woodall <william@osrfoundation.org>

* replace \r\n with \n too, because you get this in emulated tty mode

Signed-off-by: William Woodall <william@osrfoundation.org>

* fix a new test failure due a change in how pytest represents exceptions

See: pytest-dev/pytest#5579

Signed-off-by: William Woodall <william@osrfoundation.org>

* refactor to not use YAML in eval of emulate_tty option

Signed-off-by: William Woodall <william@osrfoundation.org>

* fix typo

Signed-off-by: William Woodall <william@osrfoundation.org>

* refactor emulate_tty tests and test constructor argument

Signed-off-by: William Woodall <william@osrfoundation.org>

* change default for emulate_tty to be False and fix warnings

Signed-off-by: William Woodall <william@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question general question, might be closed after 2 weeks of inactivity
Projects
None yet
Development

No branches or pull requests

6 participants