Allow for v2.0.x series compatibility with pytest-astropy by drdavella · Pull Request #6833 · astropy/astropy

drdavella · 2017-11-10T17:24:05Z

This also required a backport of the separation of Astropy's pytest plugins into individual modules.

astropy-bot · 2017-11-10T17:24:07Z

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

I noticed the following issue with this pull request:

Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

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

drdavella · 2017-11-10T17:25:33Z

+    import pytest_doctestplus
+    del pytest_doctestplus
+except ImportError:
+    pytest_plugins.append('astropy.tests.pytest_doctestplus')


The code that allows for compatibility with pytest-astropy begins here.

pllim

For stuff that requires backport, I'll defer to @bsipocz . Thanks for the quick solution!

pllim · 2017-11-10T17:33:40Z

Hmm, technically, you have a change log, so you shouldn't label it as "no change log required".

drdavella · 2017-11-10T17:35:43Z

@pllim that's true... technically the change log entry comes from a commit on v3.0, but I can just update the PR number for this commit.

On the other hand, I'm not sure that change required a change log entry to begin with...

But I'll leave the decision to someone else 🤐

bsipocz · 2017-11-10T18:11:04Z

Thanks @drdavella. I'll try to come back to this in a reasonable time, but probably only after I'm back from dotastro.

drdavella · 2017-11-10T19:58:02Z

@bsipocz thanks. There's something weird going on here, so it's going to take me a bit more time to sort it out anyway.

… packages may try and access that attribute

eteq · 2017-11-29T22:51:37Z

A quick note regarding some issues that have cropped up: when trying to use this to use the pytest plugins along side astropy, some issues appeared for me. Specifically, I get errors like those below when doing <someaffiliatedpackage>.test(). More debugging info in the Slack channel, but this should probably be resolved to make sure the new plugins don't kill affiliated packages' tests.

>>> photutils.test()
...
ValueError: option names {'--config-dir'} already added

This is to enable backwards compatibility with affiliated packages that may both use the astropy test runner and import from astropy.tests.pytest_plugins. In this case it prevents the config/cache plugins from being registered with pytest more than once.

drdavella · 2017-11-30T03:09:19Z

@eteq thanks for the report. This issue should be addressed in the latest commit.

drdavella · 2017-11-30T13:26:47Z

@bsipocz, @astrofrog, I believe the travis failures for Py27 are unrelated to this PR. The failure on CircleCI appears to be related to the use of pytest-xdist, but I'm not sure whether that's a cause for concern or not.

eteq · 2017-11-30T14:48:06Z

I've checked and this latest commit fixes the problem with the affiliated package test(), so that's great!

drdavella · 2017-11-30T15:54:06Z

@bsipocz, @eteq, @astrofrog, this is possibly a stupid question but how would we feel about making pytest-astropy a requirement for v2.0.3+? I've done most of the work already but i keep encountering strange issues in the attempt to backport compatibility. Rather than trying to support both cases, requiring pytest-astropy seems like it might be an easier solution. We could support this without API changes for affiliated packages, but they would have to install pytest-astropy if they use the test runner. The easiest thing to do for the v2.0.x series might be just to add pytest-astropy to install_requires for Astropy.

eteq · 2017-11-30T17:03:55Z

@drdavella - I'm 👎 on that because it goes against our general philosophy of no "big changes" (even if they're aren't features per se) in bugfix releases. Given the hiccups I've been encountering when going beyond the simplest cases, I think it's important we leave users/affiliated developers the option to leave things as they stand (at least until 4.0 when we stop supporting 2.x).

At some point it might be so big a maintenance burden that it's just not worth it, but it seems early to me to say that now. (Admittedly easy for me to say as I'm not the one doing most of it...)

But others might have a different opinion here.

eteq

A minor comment re: the changelog, but another thought related to the discussion above: it might be easier to maintain this if we change this to instead place all the new plugins into extern for astropy 2.0.x and control the plugin-loading on the pytest side instead of having to use astropy/tests/pytest_plugins.py to manage the loading. That will make future maintainence easier and might get around all the "two things are installed simultaneously" issues.

eteq · 2017-11-30T17:10:25Z


 - Fixed a bug that meant that the data.astropy.org mirror could not be used when
  using --remote-data=astropy. [#6724]
+- Split pytest plugins into separate modules. [#6384]


This should probably be [#6384, #6833], right? I'm thinking that'll be useful for someone who sees this and wants to know where to find more context, and this is where the "real" merge happened for this version...

this sounds sensible to me, too, though we should add the "manual backport" to #6384, too.

bsipocz

Looks good, I only have a few marginal nitpicks.

bsipocz · 2017-11-30T17:55:00Z

          env: SETUP_CMD='test --coverage --remote-data=astropy -a "--mpl"'
               CONDA_DEPENDENCIES=$CONDA_ALL_DEPENDENCIES
-               PIP_DEPENDENCIES='cpp-coveralls objgraph jplephem pytest-mpl bintrees'
+               PIP_DEPENDENCIES='cpp-coveralls objgraph jplephem pytest-astropy pytest-mpl bintrees'


pytest-mpl is already part of pytest-astropy isn't it?

bsipocz · 2017-11-30T17:55:26Z


 - Fixed a bug that meant that the data.astropy.org mirror could not be used when
  using --remote-data=astropy. [#6724]
+- Split pytest plugins into separate modules. [#6384]


nitpick, but add an empty line before this entry

bsipocz · 2017-11-30T17:56:54Z

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


nitpick as it doesn't matter here, but better to pick up the habit to use double backticks for things not sphinx linkable

bsipocz · 2017-11-30T17:57:43Z

+# command line flag. This is either passed to `pytest` directly or to the
+# `setup.py test` command.
+#
+# TODO: This import should eventually be removed once backwards compatibility


do we have an eta for that? If we have please include, it's much easier to grep for it.

bsipocz · 2017-11-30T18:01:01Z

+                FIX)
+
+        class DocTestModulePlus(doctest_plugin.DoctestModule):
+            # pytest 2.4.0 defines "collect".  Prior to that, it defined


is this collect() below then just needed as a workaround for pytest <2.4? We already require 2.8 to the 2.0.x branch, so if yes, please remove.

bsipocz · 2017-11-30T18:05:26Z

+from pkgutil import find_loader
+# This is necessary to prevent pytest from reading the doctestplus plugin when
+# the pytest_doctestplus standalone package is installed
+if find_loader('pytest_doctestplus') is None:


it's already been done in the test runner, is it necessary to repeat here, too?

if yes, why not do it for remote-data and open files, too?

bsipocz · 2017-11-30T18:06:57Z

        """
        remote_data : {'none', 'astropy', 'any'}, optional
-            Controls whether to run tests marked with @remote_data. This can be
+            Controls whether to run tests marked with @pytest.mark.remote_data. This can be


so, now both @remote_data and pytest.mark.remote_data would work? Since we don't yet rely on pytest-astropy here, probably better to leave this as is?

bsipocz · 2017-11-30T18:11:24Z

@bsipocz, @eteq, @astrofrog, this is possibly a stupid question but how would we feel about making pytest-astropy a requirement for v2.0.3+? I've done most of the work already but i keep encountering

No, we can't do that. I tend to be amongs the flexible ones, but 2.0.x is LTS, so while I may agree adding a new dependency to a normal bugfix, it's really a no for LTS and for a something that terribly break backwards compatibility for the affiliates and such.

bsipocz · 2017-11-30T18:14:29Z

having the plugins in extern looks to be a good compromise to me, too if @drdavella agrees that it helps.

drdavella · 2017-11-30T18:31:47Z

@bsipocz @eteq i'm working on a parallel PR now 🤞

bsipocz · 2017-12-05T14:40:38Z

Closing this as #6918 superseded it.

drdavella added Manual Backport testing labels Nov 10, 2017

drdavella added this to the v2.0.3 milestone Nov 10, 2017

drdavella requested review from astrofrog, bsipocz and pllim November 10, 2017 17:24

drdavella commented Nov 10, 2017

View reviewed changes

drdavella force-pushed the pytest-plugin-backport branch from 8194088 to 98bde63 Compare November 10, 2017 17:26

drdavella added the no-changelog-entry-needed label Nov 10, 2017

pllim reviewed Nov 10, 2017

View reviewed changes

drdavella removed the no-changelog-entry-needed label Nov 10, 2017

drdavella force-pushed the pytest-plugin-backport branch from 98bde63 to 561155e Compare November 10, 2017 19:01

drdavella force-pushed the pytest-plugin-backport branch 4 times, most recently from 55b4106 to 3624fda Compare November 13, 2017 20:31

Cadair and others added 9 commits November 29, 2017 12:07

Split pytest plugins out into individual modules.

6f971fa

Update change log for PR astropy#6384

1398682

Fix W391 (blank line at end of file)

fcf258b

Support compatibility with new pytest-astropy package

8b7a06e

IERS configuration should not rely on INTERNET_OFF

9d525c6

Time tests can no longer use INTERNET_OFF. Use @remote_data instead

ce7d361

Import disable_internet into astropy.tests for backwards compatibility

6e25332

Enable the new remote-data-strict option in setup.cfg

04c40f5

Run one travis test with pytest-astropy installed

be66e34

Backport fix to DocTestTextfilePlus for pytest>=3.2

428fb92

drdavella force-pushed the pytest-plugin-backport branch from 3624fda to 428fb92 Compare November 29, 2017 17:15

astrofrog and others added 2 commits November 29, 2017 14:50

Make sure DocTestTextfilePlus.obj does not raise an error since other…

489739e

… packages may try and access that attribute

Temporarily skip unrelated failing test in astropy.table.column

cb1e548

drdavella force-pushed the pytest-plugin-backport branch from c4f63cd to 4634fb2 Compare November 30, 2017 03:03

eteq reviewed Nov 30, 2017

View reviewed changes

bsipocz approved these changes Nov 30, 2017

View reviewed changes

drdavella mentioned this pull request Nov 30, 2017

Add new pytest plugins as extern packages to v2.0.x series #6918

Merged

bsipocz closed this Dec 5, 2017

drdavella deleted the pytest-plugin-backport branch December 5, 2017 22:28

Conversation

drdavella commented Nov 10, 2017

Uh oh!

astropy-bot Bot commented Nov 10, 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!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Nov 10, 2017

Uh oh!

drdavella commented Nov 10, 2017

Uh oh!

bsipocz commented Nov 10, 2017

Uh oh!

drdavella commented Nov 10, 2017

Uh oh!

eteq commented Nov 29, 2017

Uh oh!

drdavella commented Nov 30, 2017

Uh oh!

drdavella commented Nov 30, 2017

Uh oh!

eteq commented Nov 30, 2017

Uh oh!

drdavella commented Nov 30, 2017

Uh oh!

eteq commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eteq 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!

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!

bsipocz commented Nov 30, 2017

Uh oh!

bsipocz commented Nov 30, 2017

Uh oh!

drdavella commented Nov 30, 2017

Uh oh!

bsipocz commented Dec 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astropy-bot Bot commented Nov 10, 2017 •

edited

Loading

eteq commented Nov 30, 2017 •

edited

Loading