Deprecate _Result.excinfo

[goodboy]

Resolves #86

[RonnyPfannschmidt]

i believe for clarity of the warning the property should be referenced in the text somehow

[nicoddemus]

I agree with @RonnyPfannschmidt's suggestion, other than that LGTM.

[goodboy]

@RonnyPfannschmidt @nicoddemus pushed the change hopefully it's good enough 👍

[goodboy]

@nicoddemus what's the recommended way to make CI happy here?

I kind of want to keep failure on unexpected deprecation warnings but we also can't wrap pytest runs with pytest.deprecated_call()?

[RonnyPfannschmidt]

@tgoodlet i beleive we need to set up pytest awrning filters in a way that we dont error on plugy warnings and make a pytest release where we dont fail anymore

[nicoddemus]

I think @RonnyPfannschmidt's suggestion is on point. 👍

[goodboy]

@nicoddemus gonna rebase this and put in a newsfragment using the new towncrier stuff 👍

[goodboy]

@nicoddemus @RonnyPfannschmidt also do you both mind checking I did the warnings filter right?
It seems to work in CI at least.

[nicoddemus]

Awesome!

[nicoddemus]

Strange, I would expect the warnings to show only once, not for every call...

[nicoddemus]

Missing a D here 😉

[goodboy]

Wait what?

[nicoddemus]

Just checking the problematic code in pytest.

@hookimpl(hookwrapper=True)
def pytest_pyfunc_call(pyfuncitem):
    check_xfail_no_run(pyfuncitem)
    outcome = yield
    passed = outcome.excinfo is None
    if passed:
        check_strict_xfail(pyfuncitem)

becomes:

@hookimpl(hookwrapper=True)
def pytest_pyfunc_call(pyfuncitem):
    check_xfail_no_run(pyfuncitem)
    outcome = yield
    try:
        outcome.get_result()
        passed = True
    except Exception:
        passed = False
    if passed:
        check_strict_xfail(pyfuncitem)

TBH doesn't look like an improvement to me. Or am I missing something?

(To be clear, totally in favor about the deprecation of .result)

[goodboy]

@nicoddemus I also think it's a little uglier but @RonnyPfannschmidt seemed convinced in #86.

[nicoddemus]

I completely see the point of deprecating .result; it is too easy to forget to check for excinfo first to check for an exception, so definitely the user should always use .get_result() to get a result.

However for excinfo I'm not so convinced, because there's really no downside of the user checking for it or not. What do you think @RonnyPfannschmidt?

[goodboy]

@nicoddemus btw we can also chat now on the new gitter channel!

[goodboy]

@RonnyPfannschmidt still waiting on your input for this one.

[RonnyPfannschmidt]

@nicoddemus the question does become if we should expose the actual exception, from my pov, the concrete values of exception/result should be hidden away,

the following questions come up to me

1. does a single use really warrant exposing the data as is?
2. are there other uses that may fit an api that exposes an abstract success/failure? concept?

[nicoddemus]

does a single use really warrant exposing the data as is?

You mean a single use in pytest, or in a more general manner, like a single use case (verify if any exception has occurred)?

[RonnyPfannschmidt]

@nicoddemus in that case single use of the value in pytest - i believe its important to consider point 2 in relation

[nicoddemus]

A quick search shows that the only use case is indeed if a plugin is interested only if an exception has occurred or not.

TBH if we were discussing about including the API for excinfo I would agree we should look at a few more use cases first, but given we are talking about deprecating and then removing, I would like to see the use cases where keeping it is harmful (which is clearly the case of .result). It is just that so far, the few uses where checking if an exception has occurred will have to be rewritten, without a clear gain AFAICT.

[RonnyPfannschmidt]

i find that reasoning acceptable, on principle i'd still prefer if we had a success/failure indicator instead of exposing the exception

[nicoddemus]

That would work for pytest, but I did see some code in that search I posted before that were checking for a specific type of exception.

Also, we need to realize that we can always deprecate .excinfo later if we find a compelling use case to remove it.

[RonnyPfannschmidt]

@nicoddemus do you have a link, at least on github most of the results are accidentally committed tox virtualenvs

[nicoddemus]

Yeah those generate a lot of noise. I could find a few instances mostly in pytest plugins, for example:

https://github.com/vodik/pytest-exceptional/blob/c8a9901618ea860b10967db741280c64a9f3b4a7/pytest_exceptional.py#L68

https://github.com/jankatins/pytest-error-for-skips/blob/f1c7daafed26930fd2f59f3d1baf0df5fdb59ead/pytest_error_for_skips.py#L11

https://github.com/cielavenir/pytest-forcefail/blob/81ad226cae191fe0b28798739ca8dbbf09ff81d4/pytest_forcefail.py#L19

[RonnyPfannschmidt]

@nicoddemus all examples you linked use get_result and use pytest callinfo

[nicoddemus]

Oh you are right. Anyway, the code in those links will still need to be updated eventually... well I will leave up to you then, I don't have any strong arguments anymore. 👍

[RonnyPfannschmidt]

https://github.com/Authentise/pytest-raises/blob/aaa4999d3d79f3a07dfd6f8b6a6c01df827fecd2/pytest_raises/pytest_raises.py#L24
https://github.com/vsevolod-fedorov/hyperapp/blob/6e54afad8876114b2e5ad793c92ec576075ad2b8/conftest.py#L33

https://github.com/search?q=%22outcome.excinfo+is+None%22&type=Code

as per those its in actual usage (and github is just really bad to search code)

[RonnyPfannschmidt]

however it should be noted that all uses of the code would tremendously benefit from a failure indication

[RonnyPfannschmidt]

based on the information we collected i believe its only fair to keep the api,
but eventually replace it with a python3 api since exceptions can carry tracebacks there

we should also consider introducing a failure indication as an orthogonal matter

[nicoddemus]

we should also consider introducing a failure indication as an orthogonal matter

You have a suggestion? I think two read-only properties, passed and failed, might do the trick.

[RonnyPfannschmidt]

failed/passed dont quite match - its not a test - ill sleep over it

[nicoddemus]

Sounds good! 👍

[goodboy]

Closing according to the consensus that we keep .excinfo until a better failure indication API is proposed to replace it.