Allow pytest 3.x to use plugin for doctests in .rst files. - #5688

@mhvk - The claims is that it's v2.8.3, and even v2.4.0 is more that 3 years old, so 👍 to remove any compatibility with earlier version.

I don't think we ever formally supported a version of pytest that isn't the bundled one, so removing support for older versions than we bundle is fine.

OMG @mhvk, I made a note for myself to dig into DocTestTextfilePlus today but I am so glad you found a solution first, as it saved me more hours of staring at that file. Thanks! 🎉 I think such great news warrants a change log. Perhaps in 1.3.1?

As stated in this comment in 5227, either we pin pytest to 3.0.4 until pytest-dev/pytest#2118 is fixed or ignore those warnings. Is there a way for this PR to configure the CIs to use pytest 3.x to make sure everything will pass before merge? I notice that Travis CI here is still using pytest 2.8.

I tested this PR (with rebase to latest master) locally using python setup.py test --remote-data. Here is my test setup:

platform linux -- Python 3.5.2, pytest-3.0.4, py-1.4.31, pluggy-0.4.0

Running tests with Astropy version 2.0.dev17686.
Running tests in lib.linux-x86_64-3.5/astropy docs.

Date: 2017-01-12T09:48:00

Platform: Linux-2.6.32-642.6.2.el6.x86_64-x86_64-with-redhat-6.8-Santiago

Executable: .../anaconda/envs/dadf/bin/python

Full Python Version: 
3.5.2 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:53:06) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Numpy: 1.11.2
Scipy: 0.18.1
Matplotlib: 1.5.3
h5py: 2.6.0
Pandas: 0.19.0
Cython: 0.24
Using Astropy options: remote_data: any.

rootdir: /tmp/astropy-test-1xgt184n, inifile: setup.cfg
collected 9015 items 

There is a failure, as shown below, but I can't tell if it is a separate issue or not. Why would "ax.format_coord()" depends on pytest version?

____________ test_format_coord_regression ____________
tmpdir = local('/tmp/pytest-of-lim/pytest-5/test_format_coord_regression0')

    def test_format_coord_regression(tmpdir):
        # Regression test for a bug that meant that if format_coord was called by
        # Matplotlib before the axes were drawn, an error occurred.
        fig = plt.figure(figsize=(3, 3))
        ax = WCSAxes(fig, [0.1, 0.1, 0.8, 0.8])
        fig.add_axes(ax)
>       assert ax.format_coord(10, 10) == ""
E       assert '11.0 11.0 (world)' == ''
E         - 11.0 11.0 (world)

astropy/visualization/wcsaxes/tests/test_misc.py:34: AssertionError

There are also two pytest warnings, as shown below. Can we ignore this? Or somehow configure conftest.py to ignore them?

======= pytest-warning summary =======
WC1 .../test_runner.py cannot collect test class 'TestRunnerBase' because it has a __init__ constructor
WC1 .../test_runner.py cannot collect test class 'TestRunner' because it has a __init__ constructor

@mhvk, what do you think of mhvk#21 ? If you are okay with it, merging it will update this PR.

@pllim - Rather than pinning pytest version number, I would suggest to update the bundled version. While I very much support the idea of removing it asap, but we need some deprecation period as it would be rather unfair with the affiliates, they may not even be pytest 3.0x compatible themselves, to remove the bundled version straight away (and thus also bumping the default pytest version...)

I can open a PR for it later tonight or tomorrow.

When applying this to 1.3, I still get two failures (Python 2.7):

_____________________________ [doctest] angles.rst _____________________________
029     <Angle 10.2345 deg>
030     >>> Angle(['10.2345d', '-20d'])    # Array of strings
031     <Angle [ 10.2345,-20.    ] deg>
032     >>> Angle('1:2:30.43 degrees')     # Sexagesimal degrees  # doctest: +FLOAT_CMP
033     <Angle 1.0417861111111113 deg>
034     >>> Angle('1 2 0 hours')           # Sexagesimal hours  # doctest: +FLOAT_CMP
035     <Angle 1.0333333333333334 hourangle>
036     >>> Angle(np.arange(1., 8.), unit=u.deg)  # Numpy array from 1..7 in degrees
037     <Angle [ 1., 2., 3., 4., 5., 6., 7.] deg>
038     >>> Angle(u'1°2′3″')               # Unicode degree, arcmin and arcsec symbols  # doctest: +FLOAT_CMP
UNEXPECTED EXCEPTION: ValueError(u"Invalid character at col 1 in angle u'1\\xc2\\xb02\\xe2\\x80\\xb23\\xe2\\x80\\xb3'",)
Traceback (most recent call last):

  File "/usr/lib/python2.7/doctest.py", line 1315, in __run
    compileflags, 1) in test.globs

  File "<doctest angles.rst[8]>", line 1, in <module>

  File "astropy/coordinates/angles.py", line 98, in __new__
    angle, angle_unit = util.parse_angle(angle, unit)

  File "astropy/coordinates/angle_utilities.py", line 356, in parse_angle
    return _AngleParser().parse(angle, unit, debug=debug)

  File "astropy/coordinates/angle_utilities.py", line 267, in parse
    str(e), angle))

ValueError: Invalid character at col 1 in angle u'1\xc2\xb02\xe2\x80\xb23\xe2\x80\xb3'

/tmp/astropy-test-FTvJux/docs/coordinates/angles.rst:38: UnexpectedException
________________________ [doctest] compound-models.rst _________________________
203 This actually takes the generated compound model and creates a light subclass
204 of it with the desired name.  This does not impose any additional overhead.  An
205 alternative syntax, which is equivalent to what
206 `~astropy.modeling.Model.rename` is doing, is to directly use the model
207 expression as the base class of a new class::
208 
209     >>> class TwoGaussians(Gaussian1D + Gaussian1D):
210     ...     """A superposition of two Gaussians."""
211     ...
212     >>> TwoGaussians
Differences (unified diff with -expected +actual):
    @@ -1,3 +1,18 @@
    -<class '__main__.TwoGaussians'>
    -Name: TwoGaussians (CompoundModel...)
    -...
    +<class 'TwoGaussians'>
    +Name: TwoGaussians (CompoundModel252)
    +Inputs: (u'x',)
    +Outputs: (u'y',)
    +Fittable parameters: (u'amplitude_0', u'mean_0', u'stddev_0', u'amplitude_1', u'mean_1', u'stddev_1')
    +Expression: [0] + [1]
    +Components:
    +    [0]: <class 'astropy.modeling.functional_models.Gaussian1D'>
    +    Name: Gaussian1D
    +    Inputs: (u'x',)
    +    Outputs: (u'y',)
    +    Fittable parameters: ('amplitude', 'mean', 'stddev')
    +<BLANKLINE>
    +    [1]: <class 'astropy.modeling.functional_models.Gaussian1D'>
    +    Name: Gaussian1D
    +    Inputs: (u'x',)
    +    Outputs: (u'y',)
    +    Fittable parameters: ('amplitude', 'mean', 'stddev')

/tmp/astropy-test-FTvJux/docs/modeling/compound-models.rst:212: DocTestFailure

@pllim - OK, good idea to actually test whether it works on travis, so I'll merge your PR to my branch and we can check here.

@bsipocz - I agree that we cannot simply unbundle pytest, but rather have to make sure it works for whatever our bundled version is.

@olebole - are those failures under python2? I admit I didn't try that, though with @pllim's PR included, this will probably happen here too.

Python 2.7. Just added it above.

@mhvk , I killed Travis because it regressed to pytest 2.7, not the direction that we wanted:

platform linux -- Python 3.3.5 -- py-1.4.30 -- pytest-2.7.2

Appveyor uses 3.0.5 and sees one of the failures reported by @olebole in Python 2.7:

platform win32 -- Python 2.7.13, pytest-3.0.5, py-1.4.31, pluggy-0.4.0

Update: Appveyor log still shows those two pytest warnings I thought I got rid of. Maybe my solution for that does not work for Windows? ☹️

What should we do now? Wait for @bsipocz to upgrade the bundled pytest to 3.x first, then rebase and remove "use system pytest" option in Appveyor and Travis?

The failures in Python 2 appear to be real, at least based on Appveyor results.

One good news though, the WCSAxes failure is confirmed to be unrelated to pytest. It is reported separately in #5691.

I don't think we can update the bundled version because bundling pytest is no longer properly supported (at least not in the single file method we've used until now) - see #5509. I think the correct approach is to address #5509 and un-bundle pytest, or at least we can leave it for affiliated packages but not actually use it internally for the core package.

@astrofrog - Oh, right. Then we need to add a deprecation warning so affiliates will see that the previously suggested way of importing pytest from astropy is changes and they shouldn't do that any more. Can we do that in 1.3.1 or should wait for 2.0?

Update: In addition to Python 2 failures, coverage also seems to be failing with some ASCII decode error, see https://travis-ci.org/astropy/astropy/jobs/191349693

I've only had a small amount of time to look at things, but I think the unicode errors (which I can reproduce locally) are because the AstropyOutputChecker may not be used (or is not used as it should?).

I tried adding checker=AstropyOutputChecker() here but that didn't immediately solve it.

Not quite sure I'll get to this again today, but I do think we're on the right path at least...

More update: Using system pytest in Travis CI results in inconsistent pytest version being installed. For instance, the Python 2 test uses 3.0.5 but Python 3.3 test uses 2.7. Setting minversion=3 in setup.cfg or PYTEST_VERSION=3.0.5 in .travis.yml did not work. I am not sure how to force Travis to use the pytest version that we want.

@mhvk , looking at the actual doctest plugin for pytest code, I notice that the runtest() method is no longer in DoctestTextfile but instead in DoctestItem. However, our DocTestTextfilePlus still has runtest(). Could it be that we actually have to create a new DoctestItemPlus and somehow incorporate our custom runtest() there? 🤔

I thought I had avoided runtest by adding a collect method. I think with that, runtest can just be removed.

Trying to investigate a bit further, but rather hindered by finding that angles.rst and compound_models.rst do not even seem to pass on python2 if I just run current master without any changes (and use the included pytest 2.8.3), at least if I try to test them directly, with:

python setup.py test -t docs/modeling/compound-models.rst

Strangely, just running python setup.py test does work, but it looks like the documentation is not tested at all...

Hmmm, it seems none of the doctests get run! See #5695. This means the remaining issues we're having with pytest 3.x are not actually related to pytest at all -- it is just that now the tests do get run again...

@mhvk, I agree about the doctest not being run by default currently on master. Good catch! One easy solution to the Python 2 failure is to deliberately use --doctest-skip flag to skip doctest for Python 2 test suites. IMHO this is acceptable as we plan to discontinue Python 2 support in 3 years anyway. I don't think there is a way to skip a block of doctest if certain version of Python is detected (you either skip it for all settings or not).

However, despite this, the coverage failure is still a separate issue (it is run in Python 3). All the failures has a similar traceback, as follows:

______________________ ERROR collecting docs/xxx.rst _______________________
astropy/tests/pytest_plugins.py:201: in collect
    text = self.fspath.read()
/home/travis/miniconda/envs/test/lib/python3.5/site-packages/py/_path/common.py:133: in read
    return f.read()
/home/travis/miniconda/envs/test/lib/python3.5/encodings/ascii.py:26: in decode
    return codecs.ascii_decode(input, self.errors)[0]
E   UnicodeDecodeError: 'ascii' codec can't decode byte ... in position ...: ordinal not in range(128)

My original hope was that fixing coverage failure would also somehow fix failure in Python 2 but it seems unlikely.

OK, now that you figured out why the doctests were not run, we're back to master working again. So, in current master, we need the doctest_plus = enabled stanza from setup.cfg, otherwise angles.rst and compound_models.rst fail. This suggests that in pytest 3.x this line either gets ignored or does not quite have the same effect.

p.s. Do need to check that travis indeed uses the correct pytest!

EDIT: all tests pass, and travis now runs the two "standard" ones with pytest 3.x, and appveyor its python3 test.

@plim @mhvk The tests are not started anymore with python setup.py test; at least I see no difference when adding --skip-docs or not. Is there any other way to explicitely start them?

@olebole , this PR works fine on my machine (not debian though). Did you grab the latest changes to this PR and do a git clean -xdf and then a new installation before re-running the tests? Here is my log just for the wcs sub-package (I don't want to wait around for the whole thing):

$ python setup.py test -P wcs
...
=== test session starts ===
platform linux -- Python 3.5.2, pytest-3.0.4, py-1.4.31, pluggy-0.4.0

Running tests with Astropy version 2.0.dev17702.
Running tests in lib.linux-x86_64-3.5/astropy/wcs docs/wcs.

Date: 2017-01-18T10:02:47

Platform: Linux-2.6.32-642.6.2.el6.x86_64-x86_64-with-redhat-6.8-Santiago

Executable: .../anaconda/envs/dadf/bin/python

Full Python Version: 
3.5.2 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:53:06) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Numpy: 1.11.2
Scipy: 0.18.1
Matplotlib: 1.5.3
h5py: 2.6.0
Pandas: 0.19.0
Cython: 0.24
Using Astropy options: remote_data: none.

rootdir: /tmp/astropy-test-poxko_6x, inifile: setup.cfg
collected 254 items 

astropy/wcs/tests/test_pickle.py ......
astropy/wcs/tests/test_profiling.py ....................................
astropy/wcs/tests/test_utils.py .........................................
astropy/wcs/tests/test_wcs.py ......................................................
astropy/wcs/tests/test_wcsprm.py ...............................................................................................................
astropy/wcs/tests/extension/test_extension.py .
../docs/wcs/history.rst .
../docs/wcs/index.rst .
../docs/wcs/note_sip.rst .
../docs/wcs/references.rst .
../docs/wcs/relax.rst .
=== pytest-warning summary ===
WC1 None [pytest] section in setup.cfg files is deprecated, use [tool:pytest] instead.
=== 254 passed, 1 pytest-warnings in 11.86 seconds ===

@olebole - that is very strange. The travis tests do show the docs being tested in all runs. Also, I just tried on my (Debian) machine, and with --skip-docs the .rst files do get skipped (and they get included without it). It works both with the system pytest (3.0.5) and the bundled one.

@pllim I checked that I use the latest version. However, I don't use git, but applied this PR to the 1.3 release (except the travis and appveyor files). Also, I added #5689 and #5646, and unconditionally import pytest in tests/helpers.py. Did I miss an important change?
Starting from the current master is a bit difficult for me, since the Debian packaging starts from a released tarball. But I can try if needed.

I wonder if you need to apply #5678 and #5697 as well... 🤔

After applying #5678 I still have the same result. I can try the current master + #5688 but this will take some time.

@olebole - I'm still most puzzled, thinking that just using the changes to pytest_plugin.py here should allow you to run the doctests.

Indeed, I just checked and it does seem to be enough locally:

git checkout v1.3
git checkout -b v1.3-pytest-update
cp pytest_plugins_from_5688.py astropy/tests/pytest_plugins.py
export ASTROPY_USE_SYSTEM_PYTEST=1
python3 setup.py test -P wcs

This does run the doctests (but with lots of deprecation warnings: the one from setup.cfg for not using [tools:pytest] and the ones about using yield which are fixed by #5678 and #5682).

Could you point me to the full patch you are currently applying?

I uploaded now the patched version to Debian experimental. Here is the full build log (search for setup.py test to go to the tests); again the doctests are not executed.
Here are the relevant patches:

• pytest 3 + doctests: Allow pytest 3.x to use plugin for doctests in .rst files. #5688
• yield -> parametrize: [EXP] changed yield to parameterize #5678 + Use parametrize instead of yield for tests in units and io.votable #5682
• remote_data: Fix default value for remote_data option #5689

Also included from github are

• Numpy version: Ensure NUMPY_LT_1_12 works for beta prerelease. #5646
• fast-reader: Raise exception when fast_reader called with incompatible formats, guess all available fast formats #5578
• HCOMPRESS test fix: Loose identity test of a HCOMPRESSed image #4659

Additionally, there are

• use system packages -- this one also disables the ASTROPY_USE_SYSTEM_PYTEST option
• system astropy_helper
• know failures

The complete set of patches for this is here.

@plim without the first patch, doctests are executed (and fail) unless I explicitely disable them with --skip-docs. Now, they don't run, even without --skip-docs. EDIT: I just verified this by disabling the first patch locally.

@olebole , I see that some package versions are newer than what Astropy is currently supporting (e.g., matplotlib 2.0) but that should not matter, I don't think. Are you sure that distribution includes the "docs" directory? If it is not there, then the RST files won't be discovered, hence the missing doctest. Do you have a log where doctest ran successfully?

EDIT: I see that you said doctest are executed without the first patch, can you please provide a link to that log?

@olebole - Do the docstring doctests behave the same way as ones in the narrative docs in the rst files?

@pllim A log of a failed build (exactly the same source, but without the first patch) temporarily here. However, this was created locally and not by a Debian buildd, but this does not influence the test.

@olebole , I might have found the cause. Change
[tools:pytest]
to
[tool:pytest]
in the first patch and try again.

Aaargh! There was just one line I had to change manually by adding four chars, and promptly I failed.
Yes, I can confirm that this brings the doctests to run. And they run successfully. Great.

Thank you very much for the help!

@olebole , does this also fix that "remote data" problem or is that a separate issue still?

@pllim I'll check that tomorrow.

@pllim - great that you found it!! And many apologies for, likely, causing this

@astrofrog - are you happy with the changes? I think all is OK now, though I think it makes sense to raise a new issue that the code should be cleaned up since there still is cruft here for much older pytest versions.

Although a bit hidden in this PR, I want to express how great this is to get so excellent support here. From all of you! @mhvk @astrofrog @bsipocz @pllim Thank you very much!!!

Thanks! Though it definitely goes two ways -- your reports have made astropy substantially more robust!

Also, more personally, again thanks as a Debian user. E.g., just a few days ago, I realiased I needed some old midas files from 20 years, which were annoyingly still in big-endian format (made on a SparcStation...). Truly wonderful that I could convert them to fits by installing qemu-system-ppc, getting Debian on it, and then just install eso-midas!!

Nice work everyone!