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

TST: parametrize surveys to see which one fails #2904

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Dec 20, 2023

I started to see this failure in CI, the current PR refactors the test, so there is more info about which survey fails to return any results.

@szampier - is it expected that HARPS_CAT has no results? If yes, then I'll add a null result list, so we will pass the test, otherwise please let me know how to proceed.

____________________________________________________________________________________ TestEso.test_each_survey_and_SgrAstar _____________________________________________________________________________________

self = <astroquery.eso.tests.test_eso_remote.TestEso object at 0x137f03410>
tmp_path = PosixPath('/private/var/folders/9s/070g0pd502q70k3gffpxv8km0000gq/T/pytest-of-bsipocz/pytest-190/test_each_survey_and_SgrAstar0')

    def test_each_survey_and_SgrAstar(self, tmp_path):
        eso = Eso()
        eso.cache_location = tmp_path
        eso.ROW_LIMIT = 5
    
        surveys = eso.list_surveys(cache=False)
        for survey in surveys:
            print(f"Tests for {survey}")
            if survey in SGRA_SURVEYS:
                result_s = eso.query_surveys(surveys=survey, coord1=266.41681662,
                                             coord2=-29.00782497,
                                             box='01 00 00',
                                             cache=False)
                assert len(result_s) > 0
            else:
                with pytest.warns(NoResultsWarning):
                    result_s = eso.query_surveys(surveys=survey, coord1=266.41681662,
                                                 coord2=-29.00782497,
                                                 box='01 00 00',
                                                 cache=False)
                    assert result_s is None
    
                    generic_result = eso.query_surveys(surveys=survey)
>                   assert len(generic_result) > 0
E                   TypeError: object of type 'NoneType' has no len()

astroquery/eso/tests/test_eso_remote.py:183: TypeError

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9387c45) 67.30% compared to head (16b3703) 67.30%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2904   +/-   ##
=======================================
  Coverage   67.30%   67.30%           
=======================================
  Files         235      235           
  Lines       18136    18136           
=======================================
  Hits        12207    12207           
  Misses       5929     5929           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bsipocz bsipocz added this to the v0.4.7 milestone Dec 20, 2023
@bsipocz
Copy link
Member Author

bsipocz commented Dec 20, 2023

Note: the remote test is still failing, so we shall not merge it until a solution or workaround is added for it.

@szampier
Copy link
Contributor

@bsipocz it's an issue on the ESO side, HARPS_CAT should not be listed among the ESO surveys.
I spoke with @almicol about this and he will fix it in the ESO Archive.

BTW, in PR #2681 I "fixed" test_each_instrument_SgrAstar which was also failing. Now that I understand better what the test does, I'm not sure that the test makes sense although I'm not an astronomer. Essentially it checks that each ESO instrument has data around the galactic center, which is not true for many instruments. Maybe the test should check for a subset of the instruments similarly to what we do for the surveys, where we do this check for a subset of the surveys SGRA_SURVEYS.
My fix consists in ignoring NoResultsWarning which is not great. Maybe since you are working on this you could improve
test_each_instrument_SgrAstar as well or if you prefer I can try to do it.

@bsipocz bsipocz modified the milestones: v0.4.7, v0.4.8 Mar 9, 2024
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.

2 participants