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

Set appropriate stacklevel= in all calls to warnings.warn() #3403

Closed
2 tasks done
Zac-HD opened this issue Jul 15, 2022 · 9 comments · Fixed by #3406
Closed
2 tasks done

Set appropriate stacklevel= in all calls to warnings.warn() #3403

Zac-HD opened this issue Jul 15, 2022 · 9 comments · Fixed by #3406
Labels
legibility make errors helpful and Hypothesis grokable

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Jul 15, 2022

I noticed in this lovely video that the warning for st.integers().example() pointed into the Hypothesis internals, rather than at the calling user code. The solution is to set stacklevel= in:

  • SearchStrategy.example() in strategies.py
  • warn_on_missing_dtypes() in array_api.py - this might need to accept a new stacklevel= argument, since it's called from multiple different places. [turns out not to be worth the trouble 😞]

Test plan: manually check that these warnings now point to user code - the problem isn't that we break such handling but rather that we hadn't added it in the first place.

@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label Jul 15, 2022
@Zac-HD Zac-HD mentioned this issue Jul 15, 2022
20 tasks
@jameslamb
Copy link
Contributor

👋 I'm going to attempt this one.

@jameslamb
Copy link
Contributor

jameslamb commented Jul 16, 2022

I've created the following minimal, reproducible example for this.

summary of versions (click me)
platform darwin -- Python 3.9.13, pytest-7.1.2, pluggy-1.0.0
rootdir: /Users/jlamb/repos/hypothesis, configfile: pytest.ini
plugins: xdist-2.5.0, forked-1.4.0, hypothesis-6.50.1

Given a file test_warnings.py with the following.

from hypothesis import (
    example,
    given,
    strategies as st
)


def _bad_max(x, y):
    if x == 2:
        return -1
    else:
        return max(x, y)


@given(st.integers(), st.integers())
@example(2, st.integers().example())
def test_max_works(x, y):
    assert _bad_max(x, y) == max(x, y)

Running this with all of pytest's defaults

pytest ./test_warnings.py

Returns a non-0 exit code as expected, and produces output that shows user code.

================================================= FAILURES ==================================================
______________________________________________ test_max_works _______________________________________________
Traceback (most recent call last):
  File "/Users/jlamb/repos/hypothesis/test_warnings.py", line 16, in test_max_works
    @example(2, st.integers().example())
  File "/Users/jlamb/repos/hypothesis/hypothesis-python/src/hypothesis/core.py", line 1208, in wrapped_test
    raise the_error_hypothesis_found
  File "/Users/jlamb/repos/hypothesis/test_warnings.py", line 18, in test_max_works
    assert _bad_max(x, y) == max(x, y)
AssertionError: assert -1 == 3263640430859197895
 +  where -1 = _bad_max(2, 3263640430859197895)
 +  and   3263640430859197895 = max(2, 3263640430859197895)
------------------------------------------------ Hypothesis -------------------------------------------------
Falsifying explicit example: test_max_works(
    x=2, y=3263640430859197895,
)

But using warnings-as-errors, Hypothesis's warning is printed instead.

pytest -Werror ./test_warnings.py

Returns a non-0 exit code, and points to a line in the user's test file, but doesn't show the test name.

================================================== ERRORS ===================================================
_____________________________________ ERROR collecting test_warnings.py _____________________________________
Traceback (most recent call last):
  File "/Users/jlamb/repos/hypothesis/test_warnings.py", line 16, in <module>
    @example(2, st.integers().example())
  File "/Users/jlamb/repos/hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py", line 285, in example
    warnings.warn(
hypothesis.errors.NonInteractiveExampleWarning: The `.example()` method is good for exploring strategies, but should only be used interactively.  We recommend using `@given` for tests - it performs better, saves and replays failures to avoid flakiness, and reports minimal examples. (strategy: integers())
========================================== short test summary info ==========================================
ERROR test_warnings.py - hypothesis.errors.NonInteractiveExampleWarning: The `.example()` method is good f...

@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 16, 2022

Nice! I think an even simpler repro for that warning would be:

from hypothesis import strategies as st

def test():
    st.integers().example()

(in your MRE, the call to .example() happens at import time, when the arguments to the @example() decorator are evaluated)

@jameslamb
Copy link
Contributor

Cool, cool ok.

So I think that both in my example (error at test collection time) and your example (error raised directly in the test codde), hypothesis's behavior is what I'd expect.

You get a line in the traceback pointing to the specific test file + line in that test file which caused the warning.

Running your example with pytest -Werror test_warnings.py, I see this:

================================================= FAILURES ==================================================
___________________________________________________ test ____________________________________________________
Traceback (most recent call last):
  File "/Users/jlamb/repos/hypothesis/test_warnings.py", line 24, in test
    st.integers().example()
  File "/Users/jlamb/repos/hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py", line 286, in example
    warnings.warn(
hypothesis.errors.NonInteractiveExampleWarning: The `.example()` method is good for exploring strategies...

That seems pretty good to me. Can you help me understand what type of output you'd expect to see instead? I might be misunderstanding what "point to user code" means.

@jameslamb
Copy link
Contributor

Ok we talked about this in person. The issue is not about the behavior with warnings-as-errors, but in other settings where pytest just collects warnings and prints at the end of results.

For example:

"""test_warnings.py"""

from hypothesis import strategies as st


def test():
    st.integers().example()

And running like this

pytest -Wdefault ./test_warnings.py

You'll currently get output that points to the line in hypothesis where the warning was raised. That isn't enough information to understand what, in your own code, is leading to that warning.

Screen Shot 2022-07-16 at 11 37 57 AM

Instead, we'd like to have the line point to user code, e.g.

  /Users/jlamb/repos/hypothesis/test_warnings:5: NonInteractiveExampleWarning: The `.example()` method is good

@jameslamb
Copy link
Contributor

Using git grep, I found that there are a couple of other places in the library that could raise user-facing warnings.

git grep -E 'warn\('

Here's the complete list:

@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 17, 2022

I manually checked each of these while writing the list at the top of the issue 😁

@jameslamb
Copy link
Contributor

@Zac-HD I don't believe setting a single stacklevel value for _db_for_path() will work. It seems that there are multiple code paths that can result in that function getting called.

For example, given the following:

from hypothesis import given, example, strategies as st
from hypothesis.configuration import set_hypothesis_home_dir

# mkdir -p /tmp/some-nonsense
# chmod 'a=r' /tmp/some-nonsense/
set_hypothesis_home_dir("/tmp/some-nonsense")

@given(st.text())
@example("a")
def test_search_strategy_warnings(x):
    pass

Running pytest like this

pytest -Wdefault ./test_database.py

I see the following with a hard-coded stacklevel=8.

Screen Shot 2022-07-17 at 1 18 57 PM

That image suggests that for this minimal case, _db_for_path() is getting called 5 different times from 5 different code paths, each of which has a different callstack size.

Which of these would you prefer to see?

  • try to pass through stacklevel arguments from whatever code paths end up calling _db_for_path()
  • just ignore this warning for the sake of this issue
  • something else

@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 17, 2022

If it turns out to be variable, let's just ignore it for this issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants