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
BUG: Fix use with doctest + fixtures #41
Conversation
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
- Coverage 88.63% 88.45% -0.19%
==========================================
Files 31 31
Lines 1065 1074 +9
==========================================
+ Hits 944 950 +6
- Misses 121 124 +3
Continue to review full report at Codecov.
|
Thanks @larsoner ! I'll have a look tomorrow |
pytest_harvest/plugin.py
Outdated
def doctestable(): | ||
"""Do nothing, but have a doctest. | ||
|
||
Examples | ||
-------- | ||
>>> 1 + 1 | ||
2 | ||
""" | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant we have this doctest somewhere in the tests/
folder rather ? I'm a bit reluctant to leave it in the "main" part. Will it execute as a doctest if it is a dedicated test_xxx.py
file ? I guess so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if --doctests
works for things in tests/
, but I'll give it a shot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. If it ends up being overly complex to move this to tests/, another option is simply to leave it here, but to add an underscore to the name to make it private (if doctest is happy with it).
@@ -210,6 +210,13 @@ def test_synthesis_contains_everything(request): | |||
assert len(missing) == 0 | |||
|
|||
|
|||
# For some reason, adding a monkeypatch will cause an extra failure for | |||
# DoctestItem, possibly because it's a setup/teardown | |||
def test_deal_with_doctest(dummy): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please extend your comment by explaining what is the dummy
fixture here ? Maybe it is just a fixture provided by the monkeypatch
plugin but I did not find any reference to it..
Thanks @larsoner ! Looks very good to me, I made a few comments that are mostly nitpicks really, just for the sake of making maintenance a bit easier on the long run. All tests pass ok in Travis so I'll be able to merge when you'll have made the updates. |
@smarie comments addressed |
There is a wall of red here :) https://travis-ci.org/github/smarie/python-pytest-harvest/builds/727704884 Let me know if you want my help fixing it |
Hi @larsoner I merged manually and fixed the meta-tester, everything is fine now and 1.9.3 is available. There were 2 issues
Let me know if the released version is ok for you. |
@smarie yes, just |
great, thanks for the feedback and PR @larsoner ! |
Fixes #42
On
master
using--doctest-modules
with fixtures leads to:and also
master
but passes here, along with dummydoctestable
functionDoctestItem
item_obj is None
casepytest_harvest/_version.py
to.gitignore
(I assume this is useful?)FWIW locally not all tests pass actually, but the same set that fail on this PR also fail on
master
.