Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add remote_data on all tests that require network #572

Merged
merged 11 commits into from
Dec 5, 2024

Conversation

sergiopasra
Copy link
Contributor

This PR implements a first step towards the goal of running the tests of astroplan without network access. I'm using the decorator @pytest.mark.remote_data from the package pytest-remotedata, which is currently in the dependencies. In the long term, I would like to check one by one the marked tests and see if the network access is actually required or if it is an
accident, typically by using Observer.at_site

@bmorris3
Copy link
Contributor

Neat, thanks! We used to prevent problems due to internet access this with mocks like this:

astroplan/docs/conf.py

Lines 159 to 160 in 7880daa

#from astroplan.utils import _mock_remote_data
#_mock_remote_data()

Could you add the --remote-data flag to the test matrix, e.g. here:

cov: pytest --pyargs astroplan {toxinidir}/docs --cov astroplan --cov-config={toxinidir}/setup.cfg {posargs}

@sergiopasra
Copy link
Contributor Author

I have added a factor -remote that enables remote data in all tests. The tests without -remote use only local data. If you prefer that -cov tests are run always with remote data, it can be done also.

@@ -734,7 +734,7 @@ def __init__(self, min=None, max=None):
>>> from astroplan import Observer
>>> from astroplan.constraints import LocalTimeConstraint
>>> import datetime as dt
>>> subaru = Observer.at_site("Subaru", timezone="US/Hawaii")
>>> subaru = Observer.at_site("Subaru", timezone="US/Hawaii") # doctest: +REMOTE_DATA
Copy link
Member

Choose a reason for hiding this comment

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

Why is subaru needed here at all? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true, that variable is not used

@@ -127,7 +127,7 @@ def from_name(cls, query_name, name=None, **kwargs):
Examples
--------
>>> from astroplan import FixedTarget
>>> sirius = FixedTarget.from_name("Sirius")
>>> sirius = FixedTarget.from_name("Sirius") # doctest: +REMOTE_DATA
Copy link
Member

Choose a reason for hiding this comment

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

Problem with marking lines with remote data, you also need to mark all the subsequent lines derived from here onwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are some tests where I should mark also the next line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also noticed that I was using REMOTE_DATA in some tests that don't need it. I have removed them

tox.ini Outdated Show resolved Hide resolved
@pllim
Copy link
Member

pllim commented Dec 5, 2024

Thank you very much for smoking out the remote data tests, @sergiopasra ! I took some liberty to push follow up commits here. Hope you don't mind.

@pllim pllim merged commit ab94a9b into astropy:main Dec 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants