Fix line detection for properties in doctest tests #6086
Conversation
|
Is this applicable to Python itself? (then you should consider creating an issue for them, and link it here) |
|
src/_pytest/doctest.py has unexpect coverage changes (might be flaky, but looks suspicious (it did not show the lines though)) - please retry it (apply my change, and rebase). |
src/_pytest/doctest.py
Outdated
| https://bugs.python.org/issue17446 | ||
| """ | ||
| if inspect.isdatadescriptor(obj): | ||
| obj = obj.fget |
There was a problem hiding this comment.
this breaks for other descriptors
please check for property explicitly or sort it differently
There was a problem hiding this comment.
@RonnyPfannschmidt thanks!
Would be good to have this covered in a test then.
There was a problem hiding this comment.
Thank you! I've also noticed it, working on a fix.
There was a problem hiding this comment.
Use the code from the PR maybe? python/cpython#3419
There was a problem hiding this comment.
Thank you for the suggestion! Indeed, I will replace the condition with an explicit check for the property. However, I did not find a way to write a doctest test for a case when the function/method is a data descriptor, but not property.
|
It looks like the drop in coverage is because there are no tests when the line number is not detected. However, I did not found a way to write such a test. If anyone has some thoughts on this, I will appreciate suggestions! |
|
@kondratyev-nv creating a simple non-data `DistinctProperty" that wraps it could help basically anything that has a doc and is not a normal property as far as i remember |
|
@RonnyPfannschmidt To be honest, I did not completely understand you, but I've come up with kind of a strange test where line number is still not detected. Please, take a look. |
changelog/6082.bugfix.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Fix line detection for properties in doctest tests | |||
There was a problem hiding this comment.
I suggest we link to the issue:
Fix line detection for doctest samples inside ``property`` docstrings, as workaround to `bpo-17446 <https://bugs.python.org/issue17446>`__.Also good news is that there's a PR open: python/cpython#3419
Finally, could you please rebase your changes and squash all commits? Let me know if you rather us do it. 👍
There was a problem hiding this comment.
@nicoddemus Thanks! I've added a link to the issue as you suggested and squashed all commits. I saw this PR to cpython, however, it's not active since Jan 2, 2018 :(
There was a problem hiding this comment.
Sorry about that -- I just haven't been able to figure out what the reviewer wanted in it. Will try to get it in again soon.
|
@blueyed It looks like there is some error in assertions. I will fix tests and squash commits if you don't mind. |
👍 (I do not mind getting credit for this (i.e. you do not have to keep my commit separate, but you could use Co-Authored-By if you like to)) |
kondratyev-nv
force-pushed
the
master
branch
from
8ce1ce0 to
71a47de
Compare
Co-Authored-By: Daniel Hahler <[email protected]>
kondratyev-nv
force-pushed
the
master
branch
from
71a47de to
5e09797
Compare
|
Thank you very much! |
For #6082. Doctest itself does not take into account
@property. Overriding the line detection method with an additional condition onisdatadescriptorhelped.