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

Consider blacklisting (instead of whitelisting) test-failure exceptions #3462

Closed
rhaps0dy opened this issue Sep 22, 2022 · 2 comments
Closed
Labels
interop how to play nicely with other packages

Comments

@rhaps0dy
Copy link

In the pyo3 Python<->Rust bindings, a Rust panic is propagated into its Python caller as a PanicException which derives directly from BaseException. This makes some sense from the point of view of Rust semantics (panics are supposed to be caught only in rare circumstances), but it causes problems when testing PyO3 code using Hypothesis.

I believe PanicException should derive from Exception, but other libraries may do things like this in the future. Currently, the only exception not considered a test failure if everyone follows the Python docs is KeyboardInterrupt, but the exceptions to actually be caught are whitelisted.

Perhaps we could implement a blacklist instead? Something like

except BaseException as e:
    if isinstance(e, KeyboardInterrupt):
        raise e
@Zac-HD Zac-HD added the interop how to play nicely with other packages label Sep 23, 2022
@Zac-HD
Copy link
Member

Zac-HD commented Sep 23, 2022

My main concern is that this change would mean that errors people don't expect to catch would be caught, and then repeated in a loop - that can have various unfortunate side effects on the broader system. Given that you can also do:

class CatchablePanicException(PanicException, Exception): pass

@contextmanager
def catchable_panics():
    try:
        yield
    except PanicException as panic:
        catchable = CatchablePanicException(str(panic))
        catchable.__traceback__ = panic.__traceback
        # handle __notes__ too
        raise catchable from None

@given(...)
def test(x: int):
    with catchable_panics():
        something_that_might_panic(x)

to explictly convert, I'd prefer to keep the current behaviour. Thoughts?

@rhaps0dy
Copy link
Author

rhaps0dy commented Sep 23, 2022

Right, it does break behaviour. It's a little annoying to decorate all your tests with that, but I guess there's no helping it. In my case, I'll just add it to my hypothesis fork.

The sad thing about PanicException is that there's no way to get a handle on the object currently (see the missing TODO in the relevant issue ) so you can't quite do this. Instead one can do something like

except BaseException as e:
    if str(type(e)) != "PanicException":
        raise e
    # .... handle e as before

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop how to play nicely with other packages
Projects
None yet
Development

No branches or pull requests

2 participants