Add new pytest plugins as extern packages to v2.0.x series by drdavella · Pull Request #6918 · astropy/astropy

drdavella · 2017-11-30T18:44:36Z

This is an alternative approach to #6833 for providing compatibility with pytest-astropy for the v2.0.x series. It adds the new pytest plugins as extern packages. I believe this is a cleaner approach and it appears to work better 🤞.

Thanks to @eteq for the idea.

astropy-bot · 2017-11-30T18:44:38Z

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.

bsipocz · 2017-11-30T19:04:33Z

          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'


no need for pytest-mpl here

Oops, the pytest-mpl dependency actually isn't in a released version of pytest-astropy yet.

bsipocz

I find this approach much more clean and probably better maintainable.

bsipocz · 2017-11-30T19:05:10Z

 - Fixed a bug that meant that the data.astropy.org mirror could not be used when
  using --remote-data=astropy. [#6724]

+- Support compatibility with new ``pytest-astropy`` plugins. [#6918]


please add the version number of pytest-astropy that's being bundled in here

The external plugins are actually straight from the master branches of the repositories right now. Can I update and provide this with the next release of pytest-astropy?

yes, that sounds good. That case it can go to the bottom under "Other Changes"

I agree with @bsipocz here - we should tie this to a specific pytest-astropy release so affiliated package devs can be sure how to map this onto the "stand-alone" version.

drdavella · 2017-11-30T19:07:23Z

CircleCI build looks like it choked. Can someone restart?

drdavella · 2017-11-30T19:11:14Z

Ideally we would do this with submodules since it would make it easier to keep up-to-date. However, that means more infrastructure to support, which is maybe not worth it.

bsipocz · 2017-11-30T19:15:49Z

I don't think it's particularly worth doing submodules here, we just make sure to update to the latest pytest-astropy before doing a release.

drdavella · 2017-11-30T19:55:52Z

Current test failures on Py27 appear to be unrelated. Test failures against numpy-1.9 need to be addressed.

drdavella · 2017-12-01T01:47:53Z

@mhvk any ideas about how to account for the behavior of equal_nan with older versions of numpy? Originally I backported the change to use equal_nan in pytest-doctestplus, but it fails against numpy-1.9 and earlier. However, removing equal_nan means we fail against tests where nans are compared.

drdavella · 2017-12-05T18:03:39Z

        - os: linux
          env: PYTHON_VERSION=2.7 SETUP_CMD='test --remote-data=astropy -a "--mpl"'
-               CONDA_DEPENDENCIES=$CONDA_ALL_DEPENDENCIES
+               CONDA_DEPENDENCIES="`echo $CONDA_ALL_DEPENDENCIES mkl=11.3.3`"


Maybe you already tried this but I think it should be possible to just do:

CONDA_DEPENDENCIES="$CONDA_ALL_DEPENDENCIES mkl=11.3.3"

I think I tried it (I've tried a few variants), and it picked up mkl as a separate env variable

eteq · 2017-12-06T22:34:45Z

I can confirm this fixes the affiliated package test() problem, at least (which was some of the motivation for #6833). Will try to review this a bit more later, but my initial reaction is that it looks like the best solution!

eteq

Overall this looks good. I tested both <package>.test() and python setup.py test for this version with several affiliated packages and it seems to run without a hitch.

Just one new comment here to add a note in the changelog. But I think this should wait on merge until there's a specific version of pytest-astropy that it can be tied to? I think that means just tagging releases on pytest-astropy and any relevant contained packages and then updating the changelog, though, right @drdavella ?

eteq · 2017-12-07T15:23:26Z

 - Fixed a bug that meant that the data.astropy.org mirror could not be used when
  using --remote-data=astropy. [#6724]

+- Support compatibility with new ``pytest-astropy`` plugins. [#6918]


I agree with @bsipocz here - we should tie this to a specific pytest-astropy release so affiliated package devs can be sure how to map this onto the "stand-alone" version.

eteq · 2017-12-07T15:24:42Z

+
+def pytest_addoption(parser):
+
+    parser.addoption('--repeat', action='store',


This is technically a "new feature" in the 2.0.x series, right? I think that should be mentioned in the changelog. I think it's justifiable in a 2.0.x release given that it's really tied to the testing architecture, but it should be mentioned rather than silently appearing.

This is actually available in v2.0.2, so I don't think any action is required:

astropy/astropy/tests/pytest_plugins.py

Lines 112 to 113 in 93f7e75

parser.addoption('--repeat', action='store',

help='Number of times to repeat each test')

Oh my mistake. all good then

eteq · 2017-12-07T16:36:38Z

To pass on the results of some Slack discussion here: if possible we'll wait for the new pytest-astropy release to merge this so that it can include the relevant version number. But we don't want to delay this PR too long (since this fixes the tests on 2.0.x), so if that takes more than a day or so we might merge this and have a follow-on PR add the version numbers.

drdavella · 2017-12-07T21:42:29Z

@eteq, @bsipocz should be ready to go now. I released pytest-astropy-0.2.0 just now, so hopefully everything is in order.

mhvk · 2017-12-07T22:18:00Z

+                # plugin since we need to support older versions of numpy that
+                # do not provide the 'equal_nan' argument, which was added in
+                # numpy-1.10.
+                if np.isnan(ngf) and np.isnan(nwf):


This looks OK, except just to be sure: since this is for astropy itself, we can just import NUMPY_LT_1_10 from astropy.utils.compat somewhere above and do if NUMPY_LT_1_10 and np.isnan(ngf) and np.isnan(nwf):
-- that way we don't slow down the rest and also have an easier time removing this if it is no longer necessary.

drdavella · 2017-12-08T16:41:45Z

I'm not sure what's up with the current test failures on travis.

pllim · 2017-12-08T16:46:36Z

I might have an idea. Experimental PR forthcoming... ???

eteq · 2017-12-08T16:47:23Z

FWIW, I'm now happy with this once the tests are passing. (:crossed_fingers: that @pllim is fixing it...)

pllim · 2017-12-08T16:50:47Z

Ahaha... Ops. I now see the that failures here are different. I was thinking of #6942. I'll have a look here too, I guess.

pllim · 2017-12-08T16:54:57Z

Numpy is too old (1.9). broadcast_to is new in Numpy 1.10.

drdavella · 2017-12-08T17:14:45Z

Current test failures should be fixed by #6953. I'm not sure why they started showing up just now. @bsipocz did you rebase on the latest v2.0.x branch?

pllim · 2017-12-08T17:27:34Z

There is still this failure but it is not obvious to me what the fix should be -- In test_unicode_sandwich_compare[str-MaskedColumn] and test_unicode_sandwich_compare[str-Column] (not related to this PR)

>       assert np.all((obj2 == obj1) == [True, False])
E       AssertionError

drdavella · 2017-12-08T17:32:20Z

@pllim I think @bsipocz has already fixed that one in another PR: #6943

bsipocz · 2017-12-09T19:55:28Z

Thanks @drdavella! The failures are supposed to be worked around already in 2.0.x, so I took the liberty to merge this now.

bsipocz · 2017-12-11T17:48:03Z

+from ....utils.compat import NUMPY_LT_1_10
+
+
+_ALLCLOSE_KWARGS = {}


actually we shouldn't do this and any of the numpy compatibility here. anything in externs should be verbatim of an external package release (maybe with the exception of the six imports).

It's a little awkward in this case because I'm not sure we want to have to support numpy<1.10 in the plugin itself, but this was necessary in order to get the backport to work. You make a very good point, though. I'm not sure what the right solution is.

well, I'm not sure we will be able to drop that support for the LTS, so that case two version need to be maintained

Okay, I guess we might as well add this to the plugin itself then.

Cadair and others added 11 commits November 30, 2017 12:32

Split pytest plugins out into individual modules.

0e1894c

Add new pytest plugins as extern packages

aa91b0b

Remove plugins from astropy/tests that are now extern packages

0e9bdca

IERS configuration should not rely on INTERNET_OFF

626ca77

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

8c8b50e

Load extern plugins only if they are not already installed

4dc36bf

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

e579517

Import disable_internet into astropy.tests for backwards compatibility

a08f209

Run one travis test with pytest-astropy installed

0d1997d

Temporarily skip unrelated failing test in astropy.table.column

173039b

Update change log for PR astropy#6918

715d483

drdavella requested review from astrofrog, bsipocz, eteq and pllim November 30, 2017 18:44

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

bsipocz reviewed Nov 30, 2017

View reviewed changes

bsipocz approved these changes Nov 30, 2017

View reviewed changes

Bundled pytest plugins should use bundled six

5fa8f13

bsipocz added Manual Backport testing labels Nov 30, 2017

Remove newer numpy allclose argument to preserve backwards compatibility

136b997

drdavella force-pushed the pytest-plugin-extern branch from 31f3636 to 136b997 Compare December 1, 2017 01:31

Add poor-man's equal_nan for backported doctest plugin

be86889

bsipocz mentioned this pull request Dec 5, 2017

Allow for v2.0.x series compatibility with pytest-astropy #6833

Closed

trying to hack another dependency into the list

f3905c1

drdavella commented Dec 5, 2017

View reviewed changes

This was referenced Dec 5, 2017

Update testing documentation to reflect plugin changes #6811

Merged

Backport new plugin documentation to v2.0.x #6931

Merged

eteq requested changes Dec 7, 2017

View reviewed changes

Change log records the packaged version of pytest-astropy [skip ci]

fb73595

mhvk reviewed Dec 7, 2017

View reviewed changes

bsipocz removed the Manual Backport label Dec 8, 2017

addressing last review point

7d67092

bsipocz mentioned this pull request Dec 8, 2017

Fixing version dependent CI failures #6943

Merged

Make sure to use equal_nan in allclose when numpy>=1.10

c3a0d07

eteq approved these changes Dec 8, 2017

View reviewed changes

pllim mentioned this pull request Dec 8, 2017

Use broadcast_to from compat for Numpy 1.9 #6953

Merged

bsipocz merged commit 90c23cc into astropy:v2.0.x Dec 9, 2017

bsipocz mentioned this pull request Dec 9, 2017

Affiliated package test failures against v2.0.2 release #6919

Closed

drdavella deleted the pytest-plugin-extern branch December 11, 2017 14:15

bsipocz reviewed Dec 11, 2017

View reviewed changes


		def pytest_addoption(parser):

		parser.addoption('--repeat', action='store',

	parser.addoption('--repeat', action='store',
	help='Number of times to repeat each test')

		from ....utils.compat import NUMPY_LT_1_10


		_ALLCLOSE_KWARGS = {}

Conversation

drdavella commented Nov 30, 2017

Uh oh!

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

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!

drdavella commented Nov 30, 2017

Uh oh!

drdavella commented Nov 30, 2017

Uh oh!

bsipocz commented Nov 30, 2017

Uh oh!

drdavella commented Nov 30, 2017

Uh oh!

drdavella commented Dec 1, 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!

eteq commented Dec 6, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

drdavella commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drdavella commented Dec 8, 2017

Uh oh!

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

Uh oh!

Uh oh!

eteq commented Dec 8, 2017

Uh oh!

pllim commented Dec 8, 2017

Uh oh!

pllim commented Dec 8, 2017

Uh oh!

drdavella commented Dec 8, 2017

Uh oh!

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

Uh oh!

Uh oh!

drdavella commented Dec 8, 2017

Uh oh!

bsipocz commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astropy-bot Bot commented Nov 30, 2017 •

edited

Loading

drdavella commented Dec 1, 2017 •

edited

Loading

eteq commented Dec 7, 2017 •

edited

Loading

pllim commented Dec 8, 2017 •

edited

Loading

pllim commented Dec 8, 2017 •

edited

Loading