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

Closed
wants to merge 1 commit into 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

@bsipocz bsipocz force-pushed the TST_eso_parametrize_test branch from 94cf2b1 to 16b3703 Compare December 20, 2023 21:19
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.30%. Comparing base (9387c45) to head (16b3703).
Report is 620 commits behind head on main.

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
@bsipocz
Copy link
Member Author

bsipocz commented Dec 12, 2024

The module is undergoing a full refactor and the end of that process all tests will pass. So I'm closing this without merging now.

@bsipocz bsipocz closed this Dec 12, 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