Pandas: TST: pytest.raises vs. tm.assert_raises_regex

[gfyoung]

I'm wondering whether we should explicitly say in the docs that we prefer tm.assert_raises_regex over pytest.raises unless there is no error message to provide. The former is a lot more informative from a dev-perspective as to why certain code should fail.

Thoughts?

(If we can agree that we do prefer the former, I also move to convert some of the tests to use tm.assert_raises_regex instead of pytest.raises).

[jreback]

I don't think this is necessary to always check the actual content of the error, unless the content of the message is relevant (maybe to distinguish cases).

[gfyoung]

@jreback : Fair enough, but I think in many cases, we can though. One of my arguments for using tm.assert_raises_regex is that you can get a better idea of WHY code should fail beyond just the exception being raised.

So sure, I don't think all of them need to be converted, but I still do think tm.assert_raises_regex should be preferred by default. How does that sound?

[TomAugspurger]

FWIW, I would like to avoid calling pytest-specific methods where possible, to make a migration away from pytest easier in the far future, if that's ever necessary.

We could add a tm.assert_raises context manager that simply wraps pytest.raises at the moment.

[gfyoung]

@TomAugspurger : That is possible, though tm.assert_raises_regex(exc, None) also does the trick.

[jreback]

FWIW, I would like to avoid calling pytest-specific methods where possible, to make a migration away from pytest easier in the far future, if that's ever necessary.

I don't think this is a good reason. The good reason is that simply changing an error message can cause failures which are not immediately obvious (of course you want things to fail, but this mean that your regext could be too strict).

[gfyoung]

@jreback : I think @TomAugspurger was somewhat separate from the main discussion. That being said, what you said is one reason why I think we should prefer tm.assert_raises_regex by default.

[gfyoung]

I'm going to close this, since I think there is sufficient discussion on this matter (at least for myself :smile:). Can always revisit if need be.

[gfyoung]

@pandas-dev/pandas-core

I'd like to re-open this in part because pytest.raises now supports regex matching (via the match parameter), which is exactly what tm.assert_raises_regex is designed to do.

IIUC, there is some effort to make things for pytest-idiomatic. Thus, the question is: do we want to use pytest.raises across the board and drop tm.assert_raises_regex ?

I would bring up a similar question for pytest.warns, but I understand that it currently does not check stacklevel unlike our implementation --> I'll focus just on tm.assert_raises_regex.

[WillAyd]

I'd be +1 on this. Doesn't seem like a long term fit to maintain this method in our code base

[jorisvandenbossche]

Also +1 for using pytest.raises now it has all functionality (I think?) we need.

For pytest.warns, maybe it would a good idea if somebody finds the time to ask pytest if they would be interested in adding such a feature.

[TomAugspurger]

Opened https://github.com/pytest-dev/pytest/issues/4343 upstream in pytest.

On Thu, Nov 8, 2018 at 7:17 AM Joris Van den Bossche <
[email protected]> wrote:

Also +1 for using pytest.raises now it has all functionality (I think?)
we need.

For pytest.warns, maybe it would a good idea if somebody finds the time
to ask pytest if they would be interested in adding such a feature.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/pandas-dev/pandas/issues/16521#issuecomment-436990358,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQHIoEPmi7r0xtEAqmHJSUUK5p4SPgIks5utC7agaJpZM4NogdL
.

[gfyoung]

The deed has been done! See #23592.