-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Handling case when dtest is the default None #2651
Conversation
Thanks! Do you have an idea what an easy way to write a test for this would be, though? |
Not really, thus I didn't include any. I didn't find a place where where |
Thanks @bsipocz, Here's the traceback from https://circleci.com/gh/astropy/astropy/2583:
I took a quick glance at the files in https://github.com/astropy/astropy/tree/master/astropy/wcs/tests but couldn't find a possible "doctest" test in there. Can you pinpoint the file with this problem perhaps? We could implement a quick unittest for this, but if possible I would like first to understand exactly which test is causing this. |
I added an Looking around in astropy I found something interesting, it does some customization of doctests in https://github.com/astropy/astropy/blob/master/astropy/tests/pytest_doctestplus.py. I looked at that module and it seems |
We should investigate this a little more, it might be a problem or tweak in astropy's customization of the doctests. With this new information I'm a little wary of just letting |
Well, the failing doctest must be in the documentation somewhere: https://github.com/astropy/astropy/tree/master/docs/wcs |
re None: I've assumed it's a totally valid value based on this line: https://github.com/pytest-dev/pytest/blob/master/_pytest/doctest.py#L82 and below where the None case is handled properly. |
Fair enough. 😁 That part of the code has been refactored a bit, I'm not sure if that I will play around a bit with astropy's extension when I have some time, possibly tomorrow. If I can find the cause in astropy's extension and it is a simple fix I would prefer to fix it there if possible, but if not I'm OK with merging this before |
Thanks! |
@nicoddemus - We run into this with another package, too. I suspect the issue comes up for |
Thanks for the additional link @bsipocz. |
For Astropy, even this fails:
And that file has |
@pllim thanks for pointing to a working example. 👍 Running this with plain
It seems it is related to the doctest extension inside |
I spent a good hour or two trying to hack around this in |
In pytest 3.2.0 the member `dtest` is used in the `reportinfo()` function to report the location of the test examples in the doc, but the doctestplus extension does not pass one in the constructor. Ref: pytest-dev/pytest#2651
Opened a PR at astropy/astropy#6423 explaining the issue in more details, let's see how that plays out. |
Thanks for the help, @nicoddemus ! |
@bsipocz do you think we can close this given that the solution probably lies in astropy/astropy#6423? |
Sure. I'm waiting for other to review that one before merging, but it totally makes sense to have the fix there. |
In pytest 3.2.0 the member `dtest` is used in the `reportinfo()` function to report the location of the test examples in the doc, but the doctestplus extension does not pass one in the constructor. Ref: pytest-dev/pytest#2651
This should fix the handling of the default case when
dtest
is None. The bug was introduced in #2610.Here's a quick checklist that should be present in PRs:
Add a new news fragment into the changelog folder
Target: for
bugfix
,vendor
,doc
ortrivial
fixes, targetmaster
; for removals or features targetfeatures
;Make sure to include reasonable tests for your change if necessary
Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:
AUTHORS
;