Relocate generic pytest plugins to separate repositories by drdavella · Pull Request #6606 · astropy/astropy

drdavella · 2017-09-25T17:20:45Z

This builds on PR #6437, which can probably be closed now. It moves the pytest plugins into separate modules and loads them from setup.py rather than from conftest.py. As a consequence, this allows tests to run directly from pytest, which has been broken for some time.

This also removes doctestplus, openfiles, and remotedata from the core repository and moves them into new standalone repositories (pytest-doctestplus, pytest-openfiles, and pytest-remotedata). These packages are not quite ready for release, but it is possible to test against them by installing from the development branches using pip.

Related issues: #6424, #5402, #6392, #6451

astropy-bot · 2017-09-25T17:20:47Z

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

drdavella · 2017-09-25T17:32:00Z

I'll admit I'm not exactly sure how to install these plugins from git for the circleci build...

pllim · 2017-09-25T17:32:21Z

  [#6449]
+- Fixed issue with running test suite directly from ``pytest``. [#6437]
+
+astropy.time


I'll do a thorough review later. At a glance, the inclusion of extra astropy.time section seems unrelated.

Oh yeah, that was definitely an oversight. It might have happened when I rebased the old PR on the latest master (using git-rerere!!!).

Off topic: Someone needs to implement git-argharghargh to undo rerere.

astrofrog · 2017-09-25T17:35:32Z

@drdavella - add pip install commands inside .run_docker_tests.sh

drdavella · 2017-09-25T17:35:47Z

        - CONDA_DEPENDENCIES='Cython jinja2'
        - CONDA_ALL_DEPENDENCIES='Cython jinja2 scipy h5py matplotlib pyyaml scikit-image pandas pytz beautifulsoup4 ipython mpmath bleach'
-        - PIP_DEPENDENCIES=''
+        # For testing purposes, before these packages are available on pypi


The changes to this file and to appveyor.yml are temporary and should NOT be merged. They are only necessary to support testing against the new plugin packages until they can be released on pypi.

drdavella · 2017-09-25T17:45:03Z

I was getting failures from IERS locally and sort of assumed they weren't related. They're failing here now too, though. Does anyone know offhand whether this is a separate issue?

pllim · 2017-09-25T18:04:22Z

There is an extra -c somewhere though I can't find it in this PR.

-c: error: unrecognized arguments: /tmp/astropy-test-fq5yye1h/docs --doctest-rst --remote-data=astropy
  inifile: /tmp/astropy-test-fq5yye1h/setup.cfg
  rootdir: /tmp/astropy-test-fq5yye1h

drdavella · 2017-09-25T18:06:02Z

@pllim it's actually a very confusing error message: it's saying that the --doctest-rst and --remote-data options aren't available which means that those plugins were not installed properly.

The -c comes from the way that our test runner kicks off the test command as a subprocess. See line ~223 in astropy/tests/command.py.

pllim · 2017-09-25T18:14:01Z

        - CONDA_ALL_DEPENDENCIES='Cython jinja2 scipy h5py matplotlib pyyaml scikit-image pandas pytz beautifulsoup4 ipython mpmath bleach'
-        - PIP_DEPENDENCIES=''
+        # For testing purposes, before these packages are available on pypi
+        - GIT_DEPENDENCIES='git+https://github.com/astropy/pytest-doctestplus.git git+https://github.com/astropy/pytest-remotedata.git git+https://github.com/astropy/pytest-openfiles.git'


Does this even work? I never done it like this before. To install a package straight from repo, I use an incantation like https://github.com/spacetelescope/stsynphot_refactor/blob/master/.travis.yml#L103

Well, the idea was that if GIT_DEPENDENCIES gets expanded within PIP_DEPENDENCIES properly, then pip knows what to do with those git+ ... strings. But it's apparently not working so I'll have to try something else.

drdavella · 2017-09-25T18:39:36Z

@astrofrog, is git not installed in the docker image?

Collecting git+https://github.com/astropy/pytest-doctestplus.git
Cloning https://github.com/astropy/pytest-doctestplus.git to /tmp/pip-_qtrx2od-build
Error [Errno 2] No such file or directory: 'git' while executing command git clone -q https://github.com/astropy/pytest-doctestplus.git /tmp/pip-_qtrx2od-build
Cannot find command 'git'
Collecting git+https://github.com/astropy/pytest-remotedata.git
Cloning https://github.com/astropy/pytest-remotedata.git to /tmp/pip-7li799m9-build
Error [Errno 2] No such file or directory: 'git' while executing command git clone -q https://github.com/astropy/pytest-remotedata.git /tmp/pip-7li799m9-build
Cannot find command 'git'
Collecting git+https://github.com/astropy/pytest-openfiles.git
Cloning https://github.com/astropy/pytest-openfiles.git to /tmp/pip-m6ycvlpx-build
Error [Errno 2] No such file or directory: 'git' while executing command git clone -q https://github.com/astropy/pytest-openfiles.git /tmp/pip-m6ycvlpx-build
Cannot find command 'git'

astrofrog · 2017-09-25T18:53:58Z

@drdavella - that's very possible. Just add an apt install -y git in that case. We can add it to the docker image later.

drdavella · 2017-09-25T18:55:06Z

@astrofrog, I just realized that it was already in the script, but after my call to pip install. Sorry for the noise.

drdavella · 2017-09-25T21:19:14Z

 ]
+# Pytest plugins
+entry_points['pytest11'] = [
+    # Don't collide with the 'doctestplus' plugin name that will be registered


This comment is no longer relevant.

astrofrog · 2017-09-25T21:22:03Z

-            Time(Time.now().cxcsec, format='cxcsec', scale='ut1')
-    else:
-        Time(Time.now().cxcsec, format='cxcsec', scale='ut1')
+    Time(Time.now().cxcsec, format='cxcsec', scale='ut1')


Does this mean we should then add a second test with @internet_off? (once that exists)

I think so; that's why I opened this issue astropy/pytest-remotedata#3. However, it's actually not clear to me how important it is to check for the exceptional condition here.

@taldcroft - what would you recommend?

Another consideration: would we then have to make sure to have at least one test run in the matrix with --remote-data=none?

pllim · 2017-09-26T17:58:17Z

Travis traceback:

cythoning astropy/table/_np_utils.pyx to astropy/table/_np_utils.c

/home/travis/.travis/job_stages: line 57:  9225 Segmentation fault      (core dumped) $MAIN_CMD $SETUP_CMD

Appveyor traceback:

cythoning astropy\table\_np_utils.pyx to astropy\table\_np_utils.c
Command exited with code 1

CircleCI traceback:

cythoning astropy/table/_np_utils.pyx to astropy/table/_np_utils.c
Traceback (most recent call last):
  File "setup.py", line 121, in <module>
    **package_info
  File "/usr/lib/python3.5/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib/python3.5/distutils/dist.py", line 955, in run_commands
    self.run_command(cmd)
  File "/usr/lib/python3.5/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/astropy_src/astropy/tests/command.py", line 195, in run
    self._build_temp_install()
  File "/astropy_src/astropy/tests/command.py", line 245, in _build_temp_install
    self.run_command('build')
  File "/usr/lib/python3.5/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/usr/lib/python3.5/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/usr/lib/python3.5/distutils/command/build.py", line 135, in run
    self.run_command(cmd_name)
  File "/usr/lib/python3.5/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/usr/lib/python3.5/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/astropy_src/astropy_helpers/astropy_helpers/setup_helpers.py", line 252, in run
    orig_run(self)
  File "/astropy_src/astropy_helpers/astropy_helpers/commands/build_ext.py", line 301, in run
    super(build_ext, self).run()
  File "/usr/lib/python3/dist-packages/Cython/Distutils/build_ext.py", line 164, in run
    _build_ext.build_ext.run(self)
  File "/usr/lib/python3.5/distutils/command/build_ext.py", line 338, in run
    self.build_extensions()
  File "/usr/lib/python3/dist-packages/Cython/Distutils/build_ext.py", line 171, in build_extensions
    ext.sources = self.cython_sources(ext.sources, ext)
  File "/usr/lib/python3/dist-packages/Cython/Distutils/build_ext.py", line 320, in cython_sources
    full_module_name=module_name)
  File "/usr/lib/python3/dist-packages/Cython/Compiler/Main.py", line 677, in compile
    return compile_single(source, options, full_module_name)
  File "/usr/lib/python3/dist-packages/Cython/Compiler/Main.py", line 630, in compile_single
    return run_pipeline(source, options, full_module_name)
  File "/usr/lib/python3/dist-packages/Cython/Compiler/Main.py", line 447, in run_pipeline
    from . import Pipeline
  File "/usr/lib/python3/dist-packages/Cython/Compiler/Pipeline.py", line 9, in <module>
    from .Visitor import CythonTransform
  File "Cython/Compiler/Visitor.py", line 15, in init Cython.Compiler.Visitor (/build/cython-KHBh8L/cython-0.23.4/Cython/Compiler/Visitor.c:19341)
  File "/usr/lib/python3/dist-packages/Cython/Compiler/ExprNodes.py", line 3986, in <module>
    class SliceIndexNode(ExprNode):
  File "/usr/lib/python3/dist-packages/Cython/Compiler/ExprNodes.py", line 4138, in SliceIndexNode
    "SliceObject", "ObjectHandling.c", context={'access': 'Get'})
  File "Cython/Compiler/Code.py", line 286, in Cython.Compiler.Code.UtilityCodeBase.load (/build/cython-KHBh8L/cython-0.23.4/Cython/Compiler/Code.c:9076)
  File "Cython/Compiler/Code.py", line 524, in Cython.Compiler.Code.TempitaUtilityCode.__init__ (/build/cython-KHBh8L/cython-0.23.4/Cython/Compiler/Code.c:15347)
  File "Cython/Compiler/Code.py", line 516, in Cython.Compiler.Code.sub_tempita (/build/cython-KHBh8L/cython-0.23.4/Cython/Compiler/Code.c:15066)
SystemError: ../Objects/moduleobject.c:443: bad argument to internal function

drdavella · 2017-09-26T20:53:43Z

Ideally I would like to test this using pytest-astropy as a dependency in setup.py. In principle it's possible to use dependencies that are not yet published on pypi but are available on github using the dependency_links keyword to setuptools.setup. However, when I tried this it broke the build in loud and strange ways (e.g. it caused cython builds to segfault), so I am not going to try it again. This means that this PR will need to be updated again once the pytest plugins have been published on pypi.

[edit]
It may actually be worth experimenting with whether I can have the individual plugins as dependencies without using the meta package. The problem might have been the fact that the meta package itself also has github dependencies right now, and that might have been too much for setuptools to handle.

astrofrog · 2017-09-26T23:26:34Z

It might be worth releasing 'alpha' versions of the plugins before this PR is merged in any case to make sure this all works fine?

Since these changes have the pleasant side effect of making native pytest runs possible, we want to make sure we actually test this functionality.

Since many affiliated packages may import global variables directly from the display plugin, we need to retain the original import location for backwards compatibility.

drdavella · 2017-11-06T19:21:45Z

@astrofrog, @bsipocz I have tested these changes against sunpy, astroquery, ccdproc, and photutils. I believe that I have accounted for backwards compatibility for affiliated packages. Instructions for updating affiliated packages are included in a separate PR: #6811.

Cadair · 2017-11-07T11:45:30Z

I have tested these changes against sunpy.

@drdavella What did you do?! (If this involved code changes I would like to see it!)

Am I right in thinking that the fact this PR is ready to go means all the external bits are in place, so I don't have to wait for this or astropy 3.0 to start porting SunPy to the external pytest plugins?

bsipocz

Overall looks good. Most of my comments are nitpicks and not required for merging, but please do remove all six and python <3.5 related workarounds. (This module was deliberately left out of the big removal PRs).

bsipocz · 2017-11-07T11:39:28Z

+               INSTALL_CMD='python setup.py build_ext --inplace'
+               TEST_CMD='pytest --open-files --doctest-rst'
+          script:
+            - echo "$INSTALL_CMD"


Why are these echos needed here? Travis lists these env variable at the top of the log file anyway, e.g. https://travis-ci.org/astropy/astropy/jobs/298131620#L498

I'll remove these. There's also one at the bottom under script: which was not introduced by this PR.

yes, please remove that one, too. If any more verbosity is needed, one can always use the DEBUG=True setting

bsipocz · 2017-11-07T11:43:02Z

+from astropy.tests.plugins.display import PYTEST_HEADER_MODULES
+from astropy.tests.helper import enable_deprecations_as_exceptions

-from .tests.pytest_plugins import *


this is great to see gone, probably we should also do it in the package-template to make it less black boxy

bsipocz · 2017-11-07T11:44:27Z

 """
+
+# NOTE: This is retained only for backwards compatibility. Affiliated packages
+# should no longer import `disable_internet` from `astropy.tests`. It is now


It doesn't matter here as it's not processed by sphinx, but most of these backticks should be "``"

bsipocz · 2017-11-07T11:48:00Z

+    cache_dir = item.config.getoption('cache_dir') or cache_dir
+
+    # We can't really use context managers directly in py.test (although
+    # py.test 2.7 adds the capability), so this may look a bit hacky


I'm not sure I got this perfectly. Why can't we use context managers? The pytest version requirement is already >=3.1

The git blame shows that this comment is fairly old. I'm not sure whether this is worth fixing in this PR, but I'll take a look.

sure, also, this one can be definitely left for a follow-up

bsipocz · 2017-11-07T11:51:16Z

+PYTEST_HEADER_MODULES = OrderedDict([('Numpy', 'numpy'),
+                                     ('Scipy', 'scipy'),
+                                     ('Matplotlib', 'matplotlib'),
+                                     ('h5py', 'h5py'),


please do move these to the astropy specific conftest.py, so neither h5py, nor pandas are inherited by affiliated packages. Actually maybe even scipy and mpl can be moved (see astropy/pyregion#120 (comment))

I plan to open a separate issue/PR for this once this one is merged, if that's okay.

bsipocz · 2017-11-07T11:52:46Z

+    except AttributeError:
+        stdoutencoding = 'ascii'
+
+    if six.PY2:


do we need this? astropy 3.0 won't even start up with python2

Nope, it's just an oversight since this PR predates the Py3 migration.

bsipocz · 2017-11-07T11:54:50Z

+        sys.getdefaultencoding(),
+        locale.getpreferredencoding(),
+        sys.getfilesystemencoding())
+    if sys.version_info < (3, 3, 0):


remove this one, too

bsipocz · 2017-11-07T11:55:00Z

+    s += "float info: dig: {0.dig}, mant_dig: {0.dig}\n\n".format(
+        sys.float_info)
+
+    for module_display, module_name in six.iteritems(PYTEST_HEADER_MODULES):


remove six again

bsipocz · 2017-11-07T11:55:12Z

+    for op in special_opts:
+        op_value = getattr(config.option, op, None)
+        if op_value:
+            if isinstance(op_value, six.string_types):


bsipocz · 2017-11-07T11:59:09Z

 testpaths = "astropy" "docs"
 norecursedirs = "docs[\/]_build" "docs[\/]generated" "astropy[\/]extern" "astropy[\/]utils[\/]compat[\/]futures"
 doctest_plus = enabled
+doctest_norecursedirs = "LICENSE.rst" "CHANGES.rst" "examples" "astropy_helpers"


any reason not to put astropy_helpers to norecursedirs above? also why skip doctests for examples?

It's not necessary to add astropy_helpers to norecursedirs since it's already excluded from testpaths. I don't think the examples directory contains any tests or docs to be tested, but maybe we can update that in another PR.

Oh. But then would that be sensible for doctest to use the same configs? E.g. why does it run anything outside of testpaths?

Anyway, it's totally fine again to leave it for a follow-up.

I see what you're saying. I think it can safely be removed.

drdavella · 2017-11-07T13:52:53Z

@Cadair I just ran the sunpy tests in an environment where I had installed these changes to astropy, along with the pytest-astropy package.

Since this PR provides backwards compatibility, the only thing that you will need to do once it is merged is pip install pytest-astropy. The docs in #6811 outline the changes that will be required to update deprecated usages. It doesn't look like things will be too bad for sunpy, though.

Cadair · 2017-11-07T14:37:06Z

@drdavella we don't actually use any of the astropy plugins at the moment (we didn't want all of them lol) so I need to pull them in where appropriate now they are separate. Thanks a lot for all your work on this!

drdavella · 2017-11-07T14:58:14Z

@Cadair even if sunpy doesn't use any of the plugins, it is importing things from astropy.tests.pytest_plugins, so those imports will need to be fixed.

bsipocz · 2017-11-07T16:55:55Z

Thanks @drdavella!

pllim · 2017-11-07T17:11:05Z

Hey, only took 2.5 months with 95 comments. Not bad at all.

Relocate generic pytest plugins to separate repositories

drdavella added Enhancement testing labels Sep 25, 2017

drdavella requested review from astrofrog, bsipocz and pllim September 25, 2017 17:20

pllim added Refactoring Work in progress labels Sep 25, 2017

pllim added this to the v3.0.0 milestone Sep 25, 2017

pllim mentioned this pull request Sep 25, 2017

Allow test suite to run directly from pytest #6437

Closed

pllim reviewed Sep 25, 2017

View reviewed changes

drdavella commented Sep 25, 2017

View reviewed changes

pllim reviewed Sep 25, 2017

View reviewed changes

drdavella changed the title ~~WIP: Relocate generic plugins to separate repositories~~ WIP: Relocate generic pytest plugins to separate repositories Sep 25, 2017

drdavella commented Sep 25, 2017

View reviewed changes

astrofrog reviewed Sep 25, 2017

View reviewed changes

drdavella force-pushed the relocate-plugins branch 2 times, most recently from 740e5f7 to 828edd7 Compare September 26, 2017 20:14

drdavella force-pushed the relocate-plugins branch from 828edd7 to a99bfcc Compare September 27, 2017 14:38

drdavella and others added 6 commits November 6, 2017 13:28

Remove 'repeat' plugin: use pytest-repeat instead

f3e356e

Convert one travis test to use native pytest...

cee0344

Since these changes have the pleasant side effect of making native pytest runs possible, we want to make sure we actually test this functionality.

Add astropy.tests.pytest_plugins for backwards compatibility...

c16c2fa

Since many affiliated packages may import global variables directly from the display plugin, we need to retain the original import location for backwards compatibility.

Import disable_internet into astropy.tests for backwards compatibility

1919592

Add comment about backwards compatibility

93a78fb

Add pytest-astropy to pip-requirements-dev

5912e3f

drdavella force-pushed the relocate-plugins branch from 75af233 to 5912e3f Compare November 6, 2017 18:29

astrofrog approved these changes Nov 6, 2017

View reviewed changes

bsipocz requested changes Nov 7, 2017

View reviewed changes

Address feedback from code review

00e77db

bsipocz approved these changes Nov 7, 2017

View reviewed changes

bsipocz merged commit 4749e9b into astropy:master Nov 7, 2017

drdavella deleted the relocate-plugins branch November 7, 2017 16:56

drdavella mentioned this pull request Nov 7, 2017

Remove all imports of remote_data decorator #6814

Merged

This was referenced Dec 6, 2017

Running tests directly from pytest is still broken on v2.0.x #6936

Closed

Running tests directly from pytest is broken #6424

Closed

drdavella mentioned this pull request Aug 29, 2018

Timeline for removal of deprecated astropy.tests.pytest_plugins #7782

Closed

pllim mentioned this pull request Jan 18, 2019

TST: Fix import failure from astropy.tests ejeschke/ginga#738

Merged

astrofrog pushed a commit to astropy/pytest-astropy-header that referenced this pull request Oct 7, 2019

Merge pull request astropy/astropy#6606 from drdavella/relocate-plugins

ede8349

Relocate generic pytest plugins to separate repositories

Conversation

drdavella commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drdavella commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drdavella commented Sep 25, 2017

Uh oh!

pllim commented Sep 25, 2017

Uh oh!

drdavella commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drdavella commented Sep 25, 2017

Uh oh!

astrofrog commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drdavella commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Sep 26, 2017

Uh oh!

drdavella commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Sep 26, 2017

Uh oh!

drdavella commented Nov 6, 2017

Uh oh!

Cadair commented Nov 7, 2017

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

drdavella commented Sep 25, 2017 •

edited

Loading

astropy-bot Bot commented Sep 25, 2017 •

edited

Loading

pllim Sep 25, 2017 •

edited

Loading

drdavella commented Sep 25, 2017 •

edited

Loading

astrofrog commented Sep 25, 2017 •

edited

Loading

drdavella commented Sep 26, 2017 •

edited

Loading