Expand --remote-data to allow different modes by astrofrog · Pull Request #5506 · astropy/astropy

astrofrog · 2016-11-25T20:10:12Z

(This came out of a discussion in #5496)

This was a bit of work to get right, but with this pull request, it is now possible to optionally decorate remote tests using:

@remote_data(source='astropy')

Such tests are run by default but will fail if the test tries to access non-astropy data (i.e. data not on data.astropy.org). The --remote-data option can now take three values:

python setup.py test --remote-data=none
python setup.py test --remote-data=astropy
python setup.py test --remote-data=any

The first skips all tests with @remote_data, the second (which is the default) skips remote tests that don't specify source='astropy' (or specify source='any' explicitly), and the third runs all remote tests.

Note that this also preserves backward-compatibility with the plain option:

python setup.py test --remote-data

which is translated to --remote-data=any.

So this is backward-compatible and now allows tests such as those for WCSAxes to run on Travis without enabling ALL the remote tests. Since the data.astropy.org server is pretty stable (unlike e.g. VO services), I don't think this should cause any issues.

Note that this can be expanded in future to allow more modes now that it is a string option!

cc @mhvk @eteq @bsipocz @pllim @MSeifert04 @keflavich @Cadair

MSeifert04 · 2016-11-25T20:18:43Z

I think remote-data=none should be assumed by default. I don't want tests to fail just because I currently have no connection or have a connection but only with limited traffic.

Otherwise: no objections. But I haven't looked to closely on the code.

astrofrog · 2016-11-25T20:20:35Z

I think remote-data=none should be assumed by default. I don't want tests to fail just because I currently have no connection.

Ok, that would be fine by me, as long as we can set --remote-data=astropy as the default on the CI services?

astrofrog · 2016-11-25T20:32:24Z

So the failure seems to happen because data.astropy.org gets translated into an IP address at some point, so we need to also find a way to whitelist the current IP address of data.astropy.org.

pllim · 2016-11-27T15:01:33Z

astropy/tests/disable_internet.py

        if allow_astropy_data:
-            data_astropy_org_ip = socket.gethostbyname('data.astropy.org')
-            valid_hosts += ('data.astropy.org', 'astropy.stsci.edu', data_astropy_org_ip)
+            for valid_host in ('data.astropy.org', 'astropy.stsci.edu'):


Huh. I am not aware that we have astropy.stsci.edu. Is this a planned addition for 1.3?

data.astropy.org is astropy.stsci.edu :)

Hmm. Could you please email me the details? Thanks!

MSeifert04 · 2016-11-27T15:44:32Z

astropy/tests/command.py

+    before distutils/setuptools try to parse the command-line options.
+    """
+    def __init__(cls, name, bases, dct):
+        for i in range(len(sys.argv)):


The range(len(...)) is something of a do-not in my opinion, I think a more elegant way would be:

try: idx = sys.argv.index('--remote-data') except ValueError: pass else: sys.argv[idx] = '--remote-data=any'

but it just does exactly the same, so keep whichever you like more.

MSeifert04 · 2016-11-27T15:48:27Z

astropy/tests/runner.py

-            run these tests.
+        remote_data : {'none', 'astropy', 'any'}, optional
+            Controls whether to run tests marked with @remote_data. This can be
+            set to run no tests with remote data (``no``), only ones that use


no should be none

MSeifert04 · 2016-11-27T15:49:23Z

astropy/tests/pytest_plugins.py

-    parser.addoption("--remote-data", action="store_true",
+
+    # The following means that if --remote-data is not specified, the default
+    # is 'astropy', but if it is specified without arguments, it defaults to


I thought the default is none.

Ah but if you do --remote-data on its own, it does the old behavior and defaults to any

I meant the first sentence: The following means that if --remote-data is not specified, the default is 'astropy'.

Oops, fixed

MSeifert04 · 2016-11-27T15:50:44Z

astropy/tests/pytest_plugins.py

+            raise ValueError("source should be 'astropy' or 'any'")

+        if remote_data_config == 'none':
+            if source in ('astropy', 'any'):


This is always true because you already raised an exception if source not in ('astropy', 'any').

…odes. The --remote-data flag takes three possible options: 'none', 'astropy' (for data from data.astropy.org) and 'any'. If --remote-data is not specified, it defaults to 'astropy', and if it is specified without a mode, it defaults to 'any'

…ompatible

mhvk · 2016-11-29T15:13:16Z

astropy/tests/tests/test_skip_remote_data.py

-        assert True
+    if pytestconfig.getoption('remote_data') == 'none':
+        pytest.fail('@remote_data was not skipped with remote_data=none')
+    else:


This else clause is superfluous, no? The same package data gets downloaded on line 42.

Good catch, thanks!

mhvk · 2016-11-29T15:14:59Z

Had a look only now, but this looks very good. One nitpicky comment only.

mhvk · 2016-11-29T18:43:51Z

One more comment: I think this would warrant a changelog entry.

astrofrog · 2016-11-29T20:48:49Z

@mhvk - I've added a changelog entry

mhvk · 2016-11-29T21:17:22Z

OK, great. I'm happy for this to be merged once the tests have passed.

eteq · 2016-12-01T22:22:32Z

This is in principal 👍 from me, and I didn't see any issues in a quick review. But one thing I'm concerned about: how does this affect affiliated packages that may be using the remote_data machinery? Specifically, if they have set it in the setup.cfg, does this still work?

eteq · 2016-12-01T22:38:46Z

Answered my own question: if I add the following to an affiliated package's setup.cfg:

[test]
remote_data = 1

with the current master, this correctly causes the default python setup.py test to behave like --remote-data is set. With this PR, it leads to:

E               urllib.error.URLError: <urlopen error An attempt was made to connect to the internet by a test that was not marked `remote_data`. The requested host was: github.com>

But with this PR, I can get it all working again by doing

[test]
remote_data = any

So the fix is pretty easy. Moreover, I think this PR is an improvement on this behavior, because before there was no way to override the setup.cfg state, whereas now at the command line --remote-data=none turns off a default of it being on.

The question is: do we accept this subtle backwards incompatibility? I know I've talked to some people about using setup.cfg in the manner I've outlined above in their affiliated packages, but I can't find any instances actually in the wild. So maybe we just accept the possibility? Or should there be a note about this in the changelog or something?

…data = 1 in setup.cfg, but emit a deprecation warning to get them to change it.

astrofrog · 2016-12-01T23:06:29Z

@eteq - I think it should now be fixed to be backward-compatible, with a deprecation warning.

eteq · 2016-12-02T04:32:21Z

Looks great. The one remaining test is delayed (for an unclear amount of time) due to a travis OS X problem. Hard to see how that could be an issue here, so will merge to let #5496 get its test updated APAP.

MSeifert04 · 2016-12-02T10:19:03Z

I stopped the OSX build to free some workers for the other PRs.

bsipocz · 2017-02-14T23:26:31Z

@astrofrog - I'm fairly certain that this PR broke the coverage reporting (see #5701). The PR merged just before this still gets the report on coveralls, while this one looks perfectly normal on travis, doesn't trigger a report on coveralls. Any ideas?

astrofrog · 2017-02-15T01:13:44Z

@bsipocz - good catch! The issue is that the following comparison now fails:

if [[ $SETUP_CMD == 'test --coverage' ]]; then

We should just check for the presence of --coverage in SETUP_CMD.

Sorry for breaking things! :-/

astrofrog added Enhancement testing labels Nov 25, 2016

astrofrog added this to the v1.3.0 milestone Nov 25, 2016

astrofrog force-pushed the remote-astropy branch from 4ae7a83 to 79e29bd Compare November 25, 2016 20:12

astrofrog added the Ready-for-final-review label Nov 25, 2016

pllim reviewed Nov 27, 2016

View reviewed changes

MSeifert04 reviewed Nov 27, 2016

View reviewed changes

astrofrog mentioned this pull request Nov 28, 2016

Merge WCSAxes into Astropy #5496

Merged

15 tasks

astrofrog force-pushed the remote-astropy branch from 284308a to 0e227d4 Compare November 28, 2016 19:01

astrofrog added 12 commits November 29, 2016 08:57

Updated tests for skipping tests with remote data

a0e43ad

Expand tests for different remote data modes

9733c66

Fix setup.py test --remote-data option and make sure it is backward-c…

48831cb

…ompatible

Update testing docs

c3a5845

Fix defaults

59b7d84

Whitelist data.astropy.org by IP address too

6e3f497

Make --remote-data default to 'none'

007ae1c

Whitelist the STScI astropy data server

2f8c39f

Mark some tests as requiring only astropy data

5c70ef1

Allow IP address for STScI astropy data server

70bd216

Minor fixes

abe39c4

astrofrog force-pushed the remote-astropy branch from 0e227d4 to abe39c4 Compare November 29, 2016 08:58

mhvk reviewed Nov 29, 2016

View reviewed changes

Added changelog entry, and removed duplicate code

cbb5c89

bsipocz mentioned this pull request Nov 30, 2016

Different approach to remote-data tests? #3155

Closed

astrofrog added 2 commits December 1, 2016 23:02

Fix default when --remote-data is not specified

bef3448

Fix compatibility with cases where people have specified e.g. remote_…

db8cb1b

…data = 1 in setup.cfg, but emit a deprecation warning to get them to change it.

eteq merged commit 732fb94 into astropy:master Dec 2, 2016

bsipocz mentioned this pull request Dec 16, 2016

remote-data shortcut -R stopped working #5620

Merged

bsipocz mentioned this pull request Jan 6, 2017

Print remote-data value in test header #5672

Merged

bsipocz mentioned this pull request Feb 14, 2017

Coverage testing is stopped since 1.2.2 release #5701

Closed

astrofrog mentioned this pull request Feb 15, 2017

Fixing coveralls reporting #5810

Merged

astrofrog deleted the remote-astropy branch November 14, 2018 15:28

Uh oh!

Conversation

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

Uh oh!

Uh oh!

MSeifert04 commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

astrofrog commented Nov 25, 2016

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

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!

mhvk commented Nov 29, 2016

Uh oh!

mhvk commented Nov 29, 2016

Uh oh!

astrofrog commented Nov 29, 2016

Uh oh!

mhvk commented Nov 29, 2016

Uh oh!

eteq commented Dec 1, 2016

Uh oh!

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

Uh oh!

Uh oh!

astrofrog commented Dec 1, 2016

Uh oh!

eteq commented Dec 2, 2016

Uh oh!

MSeifert04 commented Dec 2, 2016

Uh oh!

bsipocz commented Feb 14, 2017

Uh oh!

astrofrog commented Feb 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astrofrog commented Nov 25, 2016 •

edited

Loading

MSeifert04 commented Nov 25, 2016 •

edited

Loading

astrofrog commented Nov 25, 2016 •

edited

Loading

eteq commented Dec 1, 2016 •

edited

Loading