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

TypeError in renaming.py #822

Closed
The-Compiler opened this issue Aug 28, 2017 · 4 comments · Fixed by #823
Closed

TypeError in renaming.py #822

The-Compiler opened this issue Aug 28, 2017 · 4 comments · Fixed by #823

Comments

@The-Compiler
Copy link
Contributor

I don't have time to dig into this right now I'm afraid, but it looks like the newest release broke pytest's build:

[...]
  File "/home/travis/build/pytest-dev/pytest/.tox/py35/lib/python3.5/site-packages/_pytest/assertion/rewrite.py", line 213, in load_module
    py.builtin.exec_(co, mod.__dict__)
  File "/home/travis/build/pytest-dev/pytest/.tox/py35/lib/python3.5/site-packages/hypothesis/strategies.py", line 1232, in <module>
    min_datetime=None, max_datetime=None,
  File "/home/travis/build/pytest-dev/pytest/.tox/py35/lib/python3.5/site-packages/hypothesis/internal/renaming.py", line 52, in accept
    'in a future version of Hypothesis.'
TypeError: unsupported operand type(s) for +=: 'NoneType' and 'str'

Travis build: https://travis-ci.org/pytest-dev/pytest/jobs/268491937#L653

Affected code which I don't really understand right now:

https://github.com/HypothesisWorks/hypothesis-python/blob/ea8f3a125adf98d90b7a9de1809f596b41880f8b/src/hypothesis/internal/renaming.py#L42-L54

@alexwlchan
Copy link
Contributor

alexwlchan commented Aug 28, 2017

This occurs if with_name_check is a function without a docstring. I can reproduce this with the following snippet, Pythons 2 and 3:

@renamed_arguments(y='x')
def f(x):
    print(x)

So I think we just need to check that the docstring is not-None, and use the empty string if not.

@alexwlchan
Copy link
Contributor

I’m surprised this wasn’t caught by the Hypothesis build, because anything we’ve decorated with that should be tested – but I wonder if something inside pytest is throwing away docstrings?

@alexwlchan
Copy link
Contributor

Okay, patch available, this was an easy spot upon code read.

@The-Compiler For your benefit: renamed_arguments is a decorator we added in 3.20.0 to rename some arguments on the datetime-related strategies. Here’s an example of it in use:

https://github.com/HypothesisWorks/hypothesis-python/blob/master/src/hypothesis/strategies.py#L1233-L1242

Because this is an internal function, and only gets applied to strategies defined in the Hypothesis codebase, there’s an implicit assumption that any function it decorates will have a non-empty docstring (as we use them to generate the strategy documentation).

I haven’t seen any similar issues reported elsewhere, so it seems likely that pytest CI is stripping docstrings from our strategies. Does that sound plausible?

(Also, has this only just started failing CI? 3.20.0 is 6 days old, and I’d expect this to start causing CI failures as soon as it was released. Has something changed in pytest recently?)

@The-Compiler
Copy link
Contributor Author

Ah, I can confirm it failed earlier, I just never noticed (I'm currently mostly ignoring mails because of exams going on). This is in a test which sets PYTHONOPTIMIZE=2 which throws away docstrings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants