-
-
Notifications
You must be signed in to change notification settings - Fork 423
Euclid: New remote tests #3407
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
Euclid: New remote tests #3407
Conversation
New remote tests for euclid
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3407 +/- ##
==========================================
- Coverage 70.51% 70.50% -0.01%
==========================================
Files 232 232
Lines 19997 19999 +2
==========================================
+ Hits 14100 14101 +1
- Misses 5897 5898 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There are a couple of comments that are blockers for the tests:
- one test is failing due to the dataset is in Angstrom --> we need to put those lines in a context manager for now and expect the warning
And could you please add back the code outputs into the docs as we should show the users what they can expect as outputs even if we don't test for exact matching.
CHANGES.rst
Outdated
- New method, get_scientific_product_list, to retrieve scientific LE3 | ||
products. [#3313] | ||
- New cross-match method [#3386] | ||
- New remote tests [#3407] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for changelog for non-user facing internals like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entry referring to the PR has been removed.
INFO: Parsing tables... [astroquery.utils.tap.core] | ||
INFO: Done. [astroquery.utils.tap.core] | ||
>>> print("Found", len(tables), "tables") # doctest: +IGNORE_OUTPUT | ||
Found 34 tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the print outputs here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we were confused, since we though that the output should not be included for remote tests. We have copied them back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries. Local test runs will know better to skip these altogether so it won't cause problems for those, and keeping them in will be better for the user (or at least we think so)
docs/esa/euclid/euclid.rst
Outdated
.. Skipping authentication requiring examples | ||
.. doctest-skip:: | ||
.. doctest-remote-data:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this example doesn't require authentication any more? It indeed seems to run for me locally except that we don’t get the expected outputs
coord = SkyCoord(ra=265.8, dec=64.1, unit=(u.degree, u.degree), frame='icrs') | ||
width = u.Quantity(0.1, u.deg) | ||
height = u.Quantity(0.1, u.deg) | ||
r = euclid.query_object(coordinate=coord, width=width, height=height, async_job=True, verbose=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails with a vounit warning, I would put that into a pytest.warns context manager here knowing that we will be able to clean it up once upstream is changing behaviour
2298837
to
0f499ba
Compare
5008d84
to
9fe4042
Compare
EUCLIDSWRQ-247 New remote tests for euclid EUCLIDSWRQ-247 New remote tests for euclid EUCLIDSWRQ-247 Include PR number
bfde5c7
to
e48e283
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed on more commit to fix all the doctest failures I saw locally.
Thanks for adding these tests!
fb505e1
to
f31df50
Compare
Dear Astroquery team,
in #3221 it was suggested to implement new remote tests for the Euclid module, since Q1 was out. We have included a new set of tests that eventually we will improve. We have also updated the documentation.
cc @esdc-esac-esa-int
jira: EUCLIDMNGT-1430