Handling case when dtest is the default None by bsipocz · Pull Request #2651 · pytest-dev/pytest

bsipocz · 2017-08-02T18:16:42Z

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 or trivial fixes, target master; for removals or features target features;
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:

Add yourself to AUTHORS;

coveralls · 2017-08-02T18:53:43Z

Coverage remained the same at 91.807% when pulling 12ec901 on bsipocz:doctest_reportinfo_dtest_None into 8969bd4 on pytest-dev:master.

The-Compiler · 2017-08-02T19:42:24Z

Thanks! Do you have an idea what an easy way to write a test for this would be, though?

bsipocz · 2017-08-02T20:53:14Z

Not really, thus I didn't include any. I didn't find a place where where runner or dtest was tested with their default value, I suspect that the inline_* methods nicely populates all those kwargs.
It also seems that reportinfo() wasn't tested before #2610?

nicoddemus · 2017-08-02T20:57:51Z

Thanks @bsipocz,

Here's the traceback from https://circleci.com/gh/astropy/astropy/2583:

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/main.py", line 110, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/main.py", line 146, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 745, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
INTERNALERROR>     _MultiCall(methods, kwargs, hook.spec_opts).execute()
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 614, in execute
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/main.py", line 169, in pytest_runtestloop
INTERNALERROR>     item.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 745, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 339, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 334, in <lambda>
INTERNALERROR>     _MultiCall(methods, kwargs, hook.spec_opts).execute()
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 613, in execute
INTERNALERROR>     return _wrapped_call(hook_impl.function(*args), self.execute)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 254, in _wrapped_call
INTERNALERROR>     return call_outcome.get_result()
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 280, in get_result
INTERNALERROR>     _reraise(*ex)  # noqa
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 265, in __init__
INTERNALERROR>     self.result = func()
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 614, in execute
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/runner.py", line 65, in pytest_runtest_protocol
INTERNALERROR>     nodeid=item.nodeid, location=item.location,
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/main.py", line 571, in location
INTERNALERROR>     location = self.reportinfo()
INTERNALERROR>   File "/usr/local/lib/python2.7/dist-packages/pytest-3.2.0-py2.7.egg/_pytest/doctest.py", line 143, in reportinfo
INTERNALERROR>     return self.fspath, self.dtest.lineno, "[doctest] %s" % self.name
INTERNALERROR> AttributeError: 'NoneType' object has no attribute 'lineno'

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.

nicoddemus · 2017-08-02T21:11:49Z

I added an assert dtest is not None in DoctestItem constructor, but all tests in pytest pass so all the "doctest forms" (docstrings and text files) tested by pytest work.

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 dtest is being correctly defined, so I'm not sure where the problem actually might be.

nicoddemus · 2017-08-02T21:13:42Z

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 dtest be None in that context as it might hide bugs in pytest's own implementation.

bsipocz · 2017-08-02T22:31:55Z

Well, the failing doctest must be in the documentation somewhere: https://github.com/astropy/astropy/tree/master/docs/wcs

bsipocz · 2017-08-02T22:48:13Z

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.
Obviously I don't know enough about the inner workings of pytest to see whether that makes much sense, so any suggestions what we need to change on our side is welcome.

nicoddemus · 2017-08-02T22:58:17Z

I've assumed it's a totally valid value based on this line

Fair enough. 😁

That part of the code has been refactored a bit, I'm not sure if that dtest=None made sense at some point (today it is always not None), but of course it is fair for someone looking at the code to assume dtest being None is fine.

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 3.2.1.

bsipocz · 2017-08-02T22:59:43Z

Thanks!

bsipocz · 2017-08-03T12:56:22Z

@nicoddemus - We run into this with another package, too. I suspect the issue comes up for rst files where there is no code snippets to tests, as e.g. this file seems to be problematic, too: https://github.com/hipspy/hips/tree/master/hips/data

https://travis-ci.org/hipspy/hips/jobs/260162492#L1211

nicoddemus · 2017-08-03T13:30:31Z

Thanks for the additional link @bsipocz.

pllim · 2017-08-03T14:37:48Z

For Astropy, even this fails:

python setup.py test -t docs/wcs/index.rst

And that file has doctest-skip-all directive, so no test is supposed to even run. I expected pytest to just collect 0 tests and quits.

nicoddemus · 2017-08-03T15:42:48Z

@pllim thanks for pointing to a working example. 👍

Running this with plain pytest works as you expect:

λ pytest index.rst
============================= test session starts =============================
platform win32 -- Python 3.5.2, pytest-3.2.1.dev2+g0a15edd5, py-1.4.34, pluggy-0.4.0
rootdir: C:\Users\bruno\pytest, inifile: tox.ini
plugins: hypothesis-3.7.0
collected 0 items
hello from hook: 0 items

======================== no tests ran in 0.32 seconds =========================

It seems it is related to the doctest extension inside astropy. I will check it later to try to understand the problem better and see if might be a simple fix to astropy's doctest plugin or if we should merge this PR.

pllim · 2017-08-03T16:20:08Z

I spent a good hour or two trying to hack around this in astropy/tests/pytest_doctestplus.py but with no success. For example, I tried defining .lineno in several locations (on separate attempts) but that did not work. I am not exactly sure where None was being passed into the pytest layer, so this PR is still currently Astropy's best hope.

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

nicoddemus · 2017-08-03T22:15:34Z

Opened a PR at astropy/astropy#6423 explaining the issue in more details, let's see how that plays out.

nicoddemus · 2017-08-03T22:24:44Z

@pllim

I am not exactly sure where None was being passed into the pytest layer

It took me a while to figure this out as well, but here doctestplus is creating a DocTestTextfilePlus passing two arguments (path and name), so dtest ends up as None (the default). 😓

pllim · 2017-08-04T14:05:56Z

Thanks for the help, @nicoddemus !

nicoddemus · 2017-08-04T18:13:49Z

@bsipocz do you think we can close this given that the solution probably lies in astropy/astropy#6423?

bsipocz · 2017-08-04T18:30:30Z

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

bsipocz added 3 commits August 2, 2017 19:10

Handling case when dtest is the default None

667c311

Adding changelog

3b4cb75

Fixing typo

12ec901

nicoddemus added type: regression indicates a problem that was introduced in a release which was working previously plugin: doctests related to the doctests builtin plugin labels Aug 2, 2017

bsipocz mentioned this pull request Aug 3, 2017

Test suite needs to be updated for py.test 3.x astropy/astropy#5277

Closed

nicoddemus mentioned this pull request Aug 3, 2017

Reimplement reportinfo() in doctestplus extension astropy/astropy#6423

Merged

bsipocz closed this Aug 4, 2017

Conversation

bsipocz commented Aug 2, 2017

Uh oh!

coveralls commented Aug 2, 2017

Uh oh!

The-Compiler commented Aug 2, 2017

Uh oh!

bsipocz commented Aug 2, 2017

Uh oh!

nicoddemus commented Aug 2, 2017

Uh oh!

nicoddemus commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Aug 2, 2017

Uh oh!

bsipocz commented Aug 2, 2017

Uh oh!

bsipocz commented Aug 2, 2017

Uh oh!

nicoddemus commented Aug 2, 2017

Uh oh!

bsipocz commented Aug 2, 2017

Uh oh!

bsipocz commented Aug 3, 2017

Uh oh!

nicoddemus commented Aug 3, 2017

Uh oh!

pllim commented Aug 3, 2017

Uh oh!

nicoddemus commented Aug 3, 2017

Uh oh!

pllim commented Aug 3, 2017

Uh oh!

nicoddemus commented Aug 3, 2017

Uh oh!

nicoddemus commented Aug 3, 2017

Uh oh!

pllim commented Aug 4, 2017

Uh oh!

nicoddemus commented Aug 4, 2017

Uh oh!

bsipocz commented Aug 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicoddemus commented Aug 2, 2017 •

edited

Loading