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

Improve the error message for HealthCheck.function_scoped_fixture #3018

Closed
Zalathar opened this issue Jun 11, 2021 · 3 comments
Closed

Improve the error message for HealthCheck.function_scoped_fixture #3018

Zalathar opened this issue Jun 11, 2021 · 3 comments
Labels
docs documentation could *always* be better legibility make errors helpful and Hypothesis grokable opinions-sought tell us what you think about these ones!

Comments

@Zalathar
Copy link
Contributor

Seeing #3017 reminded me that I had previously done some partial work on improving the error message for HealthCheck.function_scoped_fixture, but never got around to actually finishing it. So here's at least an initial write-up.


On a whim I searched GitHub and Google for “HealthCheck.function_scoped_fixture”, and found a number of cases where users did not seem to end up with a good understanding of how to deal with the problem.

For example, I saw:

  • People jumping through hoops to get rid of a function-scoped fixture, when the fixture in question was totally fine to reuse and they should have just suppressed the check.
  • People rightly identifying that they should just suppress the check in their particular case, but then feeling vaguely bad about it.

I think our current error message has a pitfalls:

  • It assumes that the user wrote the fixture and is free to modify it; in practice it's also common to use existing fixtures written by someone else (e.g. built-in pytest fixtures).
  • It assumes that using function-scoped fixtures with Hypothesis is almost certainly bad; in practice there seems to be a large subset of function-scoped fixtures that are already safely reusable across examples, typically because they set up some read-only resource. In these cases, we need to be do a better job of reassuring users that suppressing the error is sometimes totally fine.
  • It assumes that widening the scope of the fixture is a reasonable solution; in practice there seem to be a lot of cases where the fixture is both reusable and lightweight, so widening the scope isn't inherently better than just suppressing the health check.
@Zalathar Zalathar added docs documentation could *always* be better legibility make errors helpful and Hypothesis grokable labels Jun 11, 2021
@Zalathar
Copy link
Contributor Author

Current error message for reference:

# Warn about function-scoped fixtures, excluding autouse fixtures because
# the advice is probably not actionable and the status quo seems OK...
# See https://github.com/HypothesisWorks/hypothesis/issues/377 for detail.
msg = (
"%s uses the %r fixture, which is reset between function calls but not "
"between test cases generated by `@given(...)`. You can change it to "
"a module- or session-scoped fixture if it is safe to reuse; if not "
"we recommend using a context manager inside your test function. See "
"https://docs.pytest.org/en/latest/how-to/fixtures.html"
"#scope-sharing-fixtures-across-classes-modules-packages-or-session "
"for details on fixture scope."
)

@Zalathar
Copy link
Contributor Author

This is what one of my draft improvements ended up looking like:

msg = """Function-scoped fixture {1!r} used by {0}

Function-scoped fixtures are not reset between examples generated by
`@given(...)`, which is often surprising and can cause subtle test bugs.

If you were expecting the fixture to run separately for each generated example,
then unfortunately you will need to find a different way to achieve your goal
(e.g. using a similar context manager instead of a fixture).

If you are confident that your test will work correctly even though the
fixture is not reset between generated examples, you can suppress this health
check to assure Hypothesis that you understand what you are doing.
"""

I'm not entirely happy with it, but I like the general direction of where I was going. The three sentences highlight three key pieces of information:

  1. Short explanation of the problem
  2. Advice for people who are surprised by the error
  3. Reassurance that sometimes the error is bogus and it's OK to suppress it in those cases

I also like the idea of inserting hard line breaks, because relying on one auto-wrapped line ends up making things harder to read in some contexts.

@Zalathar Zalathar added the opinions-sought tell us what you think about these ones! label Jun 11, 2021
@Zalathar
Copy link
Contributor Author

An alternate take on the final paragraph that tries to be a bit more practical about when to suppress the check:

If you know that the fixture is safe to reuse across multiple generated
examples, or your test manually resets the fixture state, you can suppress
this health check to assure Hypothesis that you understand what you are doing.

@Zac-HD Zac-HD closed this as completed in 616b2f3 Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs documentation could *always* be better legibility make errors helpful and Hypothesis grokable opinions-sought tell us what you think about these ones!
Projects
None yet
Development

No branches or pull requests

1 participant