fixing astroquery.MAST remote unit tests by jaymedina · Pull Request #2174 · astropy/astroquery

jaymedina · 2021-10-07T17:37:42Z

codecov · 2021-10-07T17:44:02Z

Codecov Report

Merging #2174 (4bbcd45) into main (bc722c9) will decrease coverage by 4.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2174      +/-   ##
==========================================
- Coverage   66.11%   61.80%   -4.32%     
==========================================
  Files         418      129     -289     
  Lines       28180    16287   -11893     
==========================================
- Hits        18631    10066    -8565     
+ Misses       9549     6221    -3328

Impacted Files	Coverage Δ
astroquery/alma/core.py	`36.65% <0.00%> (-0.14%)`	⬇️
astroquery/mast/tests/test_mast_remote.py
...ry/open_exoplanet_catalogue/tests/setup_package.py
astroquery/_astropy_init.py
astroquery/solarsystem/mpc/__init__.py
astroquery/utils/tap/xmlparser/tests/__init__.py
astroquery/solarsystem/jpl/__init__.py
astroquery/jplspec/setup_package.py
astroquery/ukidss/__init__.py
astroquery/utils/tap/model/tests/test_job.py
... and 280 more

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 bc722c9...4bbcd45. Read the comment docs.

eerovaher · 2021-10-07T18:18:38Z

Linked issue here: #2172

If you wish to link an issue to your pull request then you need to use a keyword GitHub recognizes. See here: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

bsipocz · 2021-10-07T19:50:28Z

Given you added WIP in the title, I converted it to a draft PR. I'm unsubscribing now, please do ping when it's ready for review.

jaymedina · 2021-10-07T19:51:14Z

Sounds good @bsipocz

pep8speaks · 2021-10-07T19:54:34Z

Hello @jaymedina! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-13 00:32:40 UTC

jaymedina · 2021-10-14T14:51:15Z

This is almost ready to go @bsipocz . I have a question regarding some of the unit tests in Tesscut and Zcut. There are some unit tests that are intentionally trying to retrieve a table with no results, but pytest crashes on these unit tests with a NoResultsWarning. I tried adding a pytest decorator pytest.mark.filterwarnings to remedy this, but it didn't recognize the NoResultsWarning exception.

Do you know of another workaround for this or should I just remove these tests?

Edit: Some unit tests are unintentionally breaking with the NoResultsWarning, namely the ones for Zcut. I think I know why some of this data is disappearing but I'll talk to some internal folks to verify.

eerovaher · 2021-10-14T15:14:27Z

In the tests that intentionally trigger a warning it would be best to use the pytest.warns context manager, which will cause the test to fail if the expected warning is not triggered. Here is an example:

astroquery/astroquery/imcce/tests/test_skybot.py

Lines 63 to 65 in 77986a0

    
           with pytest.warns(UserWarning, match='search cone radius'): 
        
               a = core.Skybot.cone_search((100, 20), 100, 2451200, 
        
                                           get_query_payload=True)

jaymedina · 2021-10-14T15:33:20Z

That worked, thanks!

jaymedina · 2021-10-14T17:37:27Z

All tests are now passing and the PR is ready for review + merge @bsipocz . There's one test that's creating a bottleneck, I'm assuming this has always been the case. It's probably one of the download ones. If it's a concern, I can look into it.

test_mast_remote.py .....................................                                                                                          [100%]

============================================================ 37 passed in 2471.04s (0:41:11) =============================================================

cc: @tomdonaldson

bsipocz · 2021-10-14T23:19:30Z

You you fix the style breakage to make sure CI is running on this? Locally this should come back without any issues: pycodestyle astroquery --count

Also, please rebase as there shouldn't be any merge commits in there. I'll do the review once we have a passing CI.

bsipocz · 2021-10-15T18:24:07Z

@jaymedina - something went very wrong with the rebase, could you ask someone at ST to walk you through it (basically no pulls should be done, and absolutely no unrelated commits should be in the PR).

Also, the test failure is genuine, running tests locally should pass with and without --remote-data otherwise CI will certainly fail.

jaymedina · 2021-10-15T19:31:39Z

Fixed it @bsipocz - the issue was instead of running git push --force like what the documentation instructs I tried to remedy the error from just git push with a pull from origin [my branch]. Working with @tomdonaldson right now and he helped me get this sorted. Once CI tests pass this should be ready to go. Thanks!

bsipocz

Two remaining tests are failing due to undefined variables used, but the rest looks OK, with minor nitpicks.

bsipocz · 2021-11-12T22:45:14Z


    def test_observations_download_file(self, tmpdir):
-        test_obs_id = '2003600312'
+        test_obs_id = OBSID


Same NameError here

bsipocz · 2021-11-12T22:45:17Z


    def test_observations_download_products(self, tmpdir):
-        test_obs_id = '2003600312'
+        test_obs_id = OBSID


This produces a NameError. Did you plan to have OBSID as a test parameter, or reuse something from another test?

bsipocz · 2021-11-12T22:50:33Z

+        result = mast.Catalogs.query_region("158.47924 -7.30962",
+                                            radius=in_radius,
                                            catalog="Gaia")
+        row = np.where(result['source_id'] == '3774902350511581696')


Nitpick, but it's good practice to use more descriptive variable names, those are helpful when someone comes back to this code, it's enough to reach fewer lines to get the context, so e.g. here it's not a row object, but a row index.
(We're not very good with this practice in the package, I think there are still a few single-letter variables, so as I said this is very much a nitpick only :) )

bsipocz · 2021-11-12T23:30:29Z

This could use some stashing to reduce the number of commits to logical chunks, but otherwise I've switched the PRs status as it fixes all but two tests.

I'll go ahead apply my comments and do some stashing of the pep8 stuff before merging.

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

…sting

…ssertions, removing explicit table size assertions.

…n tesscut (changes to new target coordinates). commented out some unit tests that returned a NoResultsWarning

…nd unused imports

bsipocz · 2021-11-13T00:34:58Z

        assert isinstance(result, Table)
        assert len(result) == sum(products['productType'] == "SCIENCE")

+    # test downloads 150+ files, 50MB+, TODO: revise OBSID to query only a few, small files for download


The code snippet from this test takes an awfully long time, I've cancelled it after many minutes, when it already downloaded 150+ files, and 50MB+. I'll open a separate issue as a reminder that the test needs to be fixed in a way that only a very small data download should happen at test time.

bsipocz · 2021-11-13T00:40:20Z

Thanks @jaymedina! and @eerovaher for helping with the review!

eerovaher reviewed Oct 7, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py Outdated

eerovaher reviewed Oct 7, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py

bsipocz added mast testing labels Oct 7, 2021

bsipocz added this to the v0.4.4 milestone Oct 7, 2021

bsipocz added the no-changelog-entry-needed label Oct 7, 2021

jaymedina changed the title ~~fixing astroquery.MAST remote unit tests~~ [WIP] fixing astroquery.MAST remote unit tests Oct 7, 2021

bsipocz reviewed Oct 7, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py Outdated

bsipocz marked this pull request as draft October 7, 2021 19:49

jaymedina changed the title ~~[WIP] fixing astroquery.MAST remote unit tests~~ fixing astroquery.MAST remote unit tests Oct 14, 2021

eerovaher reviewed Oct 14, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py

eerovaher reviewed Oct 14, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py

eerovaher reviewed Oct 14, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py Outdated

eerovaher reviewed Oct 14, 2021

View reviewed changes

Comment thread astroquery/mast/tests/test_mast_remote.py Outdated

jaymedina force-pushed the mast-unit-tests-fix branch from 16eb567 to 5a05168 Compare October 15, 2021 19:29

bsipocz marked this pull request as ready for review November 12, 2021 22:30

bsipocz approved these changes Nov 12, 2021

View reviewed changes

jaymedina and others added 16 commits November 12, 2021 16:31

test passing for test_observations_get_product_list

3571a60

testing passing for test_observation_download_products

f82be28

using sets for comparison between two data structures

7a1480d

too many output results, limiting output by shrinking radius to 0.1

33f5956

Update astroquery/mast/tests/test_mast_remote.py

84a6856

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

taking out explicit assertions and replacing with other methods of te…

5038c37

…sting

some bug fixes to test_catalogs_query_region

2c0dfe7

finished unit test fixes for observations and catalogs. adding some a…

dee882d

…ssertions, removing explicit table size assertions.

fixing max result warning in test_catalogs_query_region. fixed bugs i…

4a90e8c

…n tesscut (changes to new target coordinates). commented out some unit tests that returned a NoResultsWarning

adding pytest.warns to method calls that return no results

0c43661

filenames1 and 2 are the productFilename column of the output

9d8a20e

simpler assertions for test_catalogs_query_object

f5aa034

pep8

f607ed6

hardcoding OBSID

6b2bd4c

Cleanup asserts

96613d1

Cleaning up test file, move global variable, remove duplicated test a…

4bbcd45

…nd unused imports

bsipocz force-pushed the mast-unit-tests-fix branch from e0fe77f to 4bbcd45 Compare November 13, 2021 00:32

bsipocz reviewed Nov 13, 2021

View reviewed changes

This was referenced Nov 13, 2021

Fix MAST download products tests to limit test download to a minimum amount #2209

Closed

Failing remote tests Q4 2021 #2203

Closed

bsipocz merged commit 9862845 into astropy:main Nov 13, 2021

Uh oh!

Uh oh!

Conversation

jaymedina commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

eerovaher commented Oct 7, 2021

Uh oh!

Uh oh!

bsipocz commented Oct 7, 2021

Uh oh!

jaymedina commented Oct 7, 2021

Uh oh!

pep8speaks commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-11-13 00:32:40 UTC

Uh oh!

jaymedina commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerovaher commented Oct 14, 2021

Uh oh!

jaymedina commented Oct 14, 2021

Uh oh!

jaymedina commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bsipocz commented Oct 14, 2021

Uh oh!

bsipocz commented Oct 15, 2021

Uh oh!

jaymedina commented Oct 15, 2021

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

bsipocz Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

bsipocz Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bsipocz commented Nov 12, 2021

Uh oh!

bsipocz Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Nov 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaymedina commented Oct 7, 2021 •

edited

Loading

codecov Bot commented Oct 7, 2021 •

edited

Loading

pep8speaks commented Oct 7, 2021 •

edited

Loading

jaymedina commented Oct 14, 2021 •

edited

Loading

jaymedina commented Oct 14, 2021 •

edited

Loading