Skip to content

TST: Mark coordinates tests as requiring remote data#8033

Merged
pllim merged 2 commits intoastropy:masterfrom
pllim:fix-warn-coord-2
Dec 20, 2018
Merged

TST: Mark coordinates tests as requiring remote data#8033
pllim merged 2 commits intoastropy:masterfrom
pllim:fix-warn-coord-2

Conversation

@pllim
Copy link
Member

@pllim pllim commented Oct 30, 2018

As part of #7928 but more controversial than #7989. This one is to address the following warnings I get when I run python setup.py test -P coordinates (without --remote-data).

An attempt was made to connect to the internet by a test that was not marked `remote_data`.
The requested host was: maia.usno.navy.mil

I looked at one of the tests and traced that one to such a call that invokes IERS, which in turns invokes Internet connection.

astropy/coordinates/builtin_frames/utils.py:40: in get_polar_motion
    xp, yp, status = iers.IERS_Auto.open().pm_xy(time, return_status=True)

I am not sure if this is a proper way to address such warnings but at least it is how I could get rid of them with minimal change. With this patch, here is a log of python setup.py test -P coordinates showing that the tests are now skipped without --remote-data:
ztstlog.txt

@astropy-bot
Copy link

astropy-bot bot commented Oct 30, 2018

Check results are now reported in the status checks at the bottom of this page.

@bsipocz bsipocz added this to the v2.0.10 milestone Oct 30, 2018
@pllim pllim modified the milestones: v2.0.10, v2.0.11 Nov 11, 2018
@codecov
Copy link

codecov bot commented Nov 11, 2018

Codecov Report

Merging #8033 into master will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #8033     +/-   ##
=========================================
- Coverage   86.92%   86.73%   -0.2%     
=========================================
  Files         384      384             
  Lines       57995    57995             
  Branches     1060     1060             
=========================================
- Hits        50413    50302    -111     
- Misses       6967     7078    +111     
  Partials      615      615
Impacted Files Coverage Δ
...dinates/builtin_frames/cirs_observed_transforms.py 53.84% <0%> (-46.16%) ⬇️
astropy/coordinates/builtin_frames/itrs.py 80% <0%> (-20%) ⬇️
...builtin_frames/intermediate_rotation_transforms.py 83.01% <0%> (-16.99%) ⬇️
...coordinates/builtin_frames/icrs_cirs_transforms.py 78.1% <0%> (-14.6%) ⬇️
astropy/coordinates/sky_coordinate.py 82.28% <0%> (-8.4%) ⬇️
astropy/coordinates/earth.py 75.09% <0%> (-3.35%) ⬇️
astropy/coordinates/attributes.py 92.14% <0%> (-2.86%) ⬇️
astropy/coordinates/solar_system.py 70.06% <0%> (-1.28%) ⬇️
astropy/coordinates/baseframe.py 92.93% <0%> (-0.5%) ⬇️
astropy/coordinates/transformations.py 86.26% <0%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52d1c24...942ed54. Read the comment docs.

@pllim
Copy link
Member Author

pllim commented Nov 11, 2018

This is not release critical, re-milestoning.

@bsipocz
Copy link
Member

bsipocz commented Dec 14, 2018

@pllim - can you rebase? I need to do a new release soonish and can't see a reason why this should be kept open rather than merged in.

@pllim
Copy link
Member Author

pllim commented Dec 14, 2018

@bsipocz , it is rebased but now I wonder how much of it was a symptom that has since been fixed by #8163 . I don't have time to look into this now. Maybe we should not rush this one and it is not critical for release. Thanks!

@pllim
Copy link
Member Author

pllim commented Dec 18, 2018

@bsipocz

UPDATE: I reran the tests with and without this patch and confirmed that this is still necessary to silence those warnings. In fact, I caught one more (probably it was added since I originally ran this). I think this is good to merge if you don't see anything wrong with it.

altazframe = AltAz(location=EarthLocation(lat=0*u.deg, lon=0*u.deg, height=0*u.m),
obstime=Time('J2000'))
return fullstack_icrs.transform_to(altazframe)
with warnings.catch_warnings(): # Ignore remote_data warning
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fixture-fu is not great, so if there is a better way to do this, please let me know.


@pytest.mark.xfail
@pytest.mark.remote_data
@pytest.mark.xfail(reason="Current output is completely incorrect")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch. any chance to fix this @adrn/@eteq? I mean in a new PR not in this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made an issue to remind us #8296

@pllim
Copy link
Member Author

pllim commented Dec 18, 2018

p.s. I am not convinced that this needs to be backported all the way to 2.x. Please remilestone to something less painful if straight backport is not possible.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks uncontroversial to me, but keep it open until I actually do the release to give a bit more time to @adrn or @eteq to give their OK, too.

@pllim
Copy link
Member Author

pllim commented Dec 20, 2018

Looks like this is now needed because of IERS being down and is holding up other PRs because the test is running when it is not supposed to without this patch. Merging.

bsipocz pushed a commit that referenced this pull request Dec 24, 2018
TST: Mark coordinates tests as requiring remote data
bsipocz pushed a commit that referenced this pull request Dec 24, 2018
TST: Mark coordinates tests as requiring remote data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants