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.

Sign up for GitHub

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

Jump to bottom

Fix heasarc tests #2842

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Fix heasarc tests #2842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0d2b86
fix column name in test_heasarc_remote_isdc.test_mission_cols
zoghbi-a Sep 25, 2023
dd3a1b0
in heasarc.test, move patch_get to class level to allow for other cla…
zoghbi-a Sep 25, 2023
f44a0c3
fix flake8 E303 error
zoghbi-a Sep 25, 2023
c4f6ff4
attempting to fix the tables
zoghbi-a Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def save_response_of_get(session, method, url, params=None, timeout=10, **kwargs
return MockResponse(text)


@pytest.fixture(autouse=True)
@pytest.fixture()
def patch_get(request):
"""
If the mode is not remote, patch `requests.Session` to either return saved local data
Expand Down
2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/404652d5.dat
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/4511b20b.dat
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/6ad7a587.dat
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/6df5af53.dat
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/f6d793e8.dat
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/ff1f8d6a.dat
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion astroquery/heasarc/tests/data/last-month-integral_rev3_scw.dat
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions astroquery/heasarc/tests/test_heasarc_remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@

@parametrization_local_save_remote
class TestHeasarc:

@pytest.fixture(autouse=True)
def _patch_get(self, patch_get):
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think the original issue is that the mock should not have a remote conditional in it at the first place, the idea with all the remote tests is that they are used the same way a user would (aka without the mock patch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think the original issue is that the mock should not have a remote conditional in it at the first place, the idea with all the remote tests is that they are used the same way a user would (aka without the mock patch)

I am working on extending the heasarc services, and when I added a new test class, it was using patch_get because it had autouse=True, so I moved the autouse=True part to the classes that need it. The class I am creating does not need it. Is there a better way to do it?

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 changed this to WIP because I see errors in my local testing and I thought I am fixing them, but they don't show up in the CI.

Copy link
Member

Choose a reason for hiding this comment

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

re autouse: yes, removing it sounds good, most modules don't have autouse.

CI: we don't run the remote tests in CI on PRs, only once a week from cron, so that's why you may not see them failing. E.g. here I see the same heasarc failure as I see locally: https://github.com/astropy/astroquery/actions/runs/6279884195/job/17056292621#step:5:4890

Copy link
Member

Choose a reason for hiding this comment

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

That being said, any improvements to the module is more than welcome, even to the point of a total gut out and starting from new (e.g. that's what I'm doing with the IRSA one, switching it altogether to VO backends). For heasarc, there are a few stalled PRs, too, but we can close them off if needed.

So, feel free to ping me on slack if there are any questions or if I can help with any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re autouse: yes, removing it sounds good, most modules don't have autouse.

CI: we don't run the remote tests in CI on PRs, only once a week from cron, so that's why you may not see them failing. E.g. here I see the same heasarc failure as I see locally: https://github.com/astropy/astroquery/actions/runs/6279884195/job/17056292621#step:5:4890

I see. That makes some sense. From that link I see both local and remote tests fail. Does test_mission_cols[local] mean it is not using remote service?

../../.tox/py311-test-alldeps-devdeps-online/lib/python3.11/site-packages/astroquery/heasarc/tests/test_heasarc_remote_isdc.py::TestHeasarcISDC::test_mission_cols[local] FAILED [ 23%]
../../.tox/py311-test-alldeps-devdeps-online/lib/python3.11/site-packages/astroquery/heasarc/tests/test_heasarc_remote_isdc.py::TestHeasarcISDC::test_mission_cols[save] SKIPPED [ 23%]
../../.tox/py311-test-alldeps-devdeps-online/lib/python3.11/site-packages/astroquery/heasarc/tests/test_heasarc_remote_isdc.py::TestHeasarcISDC::test_mission_cols[remote] RERUN [ 23%]
../../.tox/py311-test-alldeps-devdeps-online/lib/python3.11/site-packages/astroquery/heasarc/tests/test_heasarc_remote_isdc.py::TestHeasarcISDC::test_mission_cols[remote] FAILED [ 23%]

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think I overgeneralized above, in all the other modules, mocked and other local tests are separated from the remote ones, here it seems that everything has a local and a remote version. According to that, test_mission_cols[local] should not use remote service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it obvious to you why test_mission_cols[local] is failing in that link, but not in the CI test for the this PR?

Copy link
Member

Choose a reason for hiding this comment

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

guesswork: I would think the canned version of that test integral query is outdated. And there is at least one bug (I guess more), with how the tests are run. E.g. a previous test method downloads the current version into the temp data dir (at least it doesn't go into the central cache location, that has been fixed already), and later tests pick that up instead of the canned version, even for the mocked version of the test.
Historically, I think this is one reason why we kept local and remote tests separate, less headache with what the given instances see, etc. (and we also don't need to fully duplicate everything local to remote).

E.g., if I remove all that uses integral_rev3_scw, it's not failing any more:

astroquery/heasarc/tests/test_heasarc_remote_isdc.py .s..s..s..s..sF.s..s..s.                                                         [100%]

================================================================= FAILURES ==================================================================
_________________________________________________ TestHeasarcISDC.test_mission_cols[remote] _________________________________________________

self = <astroquery.heasarc.tests.test_heasarc_remote_isdc.TestHeasarcISDC object at 0x129c28670>

    def test_mission_cols(self):
        heasarc = Heasarc()
        mission = 'integral_rev3_scw'
    
        with self.isdc_context:
            cols = heasarc.query_mission_cols(mission=mission)
    
        assert len(cols) == 35
    
        # Test that the cols list contains known names
        assert 'SCW_ID' in cols
        assert 'GOOD_ISGRI' in cols
        assert 'RA_X' in cols
        assert 'DEC_X' in cols
>       assert '_SEARCH_OFFSET' in cols
E       AssertionError: assert '_SEARCH_OFFSET' in ['SCW_ID', 'SCW_VER', 'SCW_TYPE', 'RA_X', 'DEC_X', 'OBS_ID', ...]

astroquery/heasarc/tests/test_heasarc_remote_isdc.py:133: AssertionError
========================================================== short test summary info ==========================================================
FAILED astroquery/heasarc/tests/test_heasarc_remote_isdc.py::TestHeasarcISDC::test_mission_cols[remote] - AssertionError: assert '_SEARCH_OFFSET' in ['SCW_ID', 'SCW_VER', 'SCW_TYPE', 'RA_X', 'DEC_X', 'OBS_ID', ...]
============================================ 1 failed, 33 passed, 17 skipped in 61.12s (0:01:01) ============================================

Copy link
Member

Choose a reason for hiding this comment

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

This is something I've been trying to work around also when experimenting with trying to switch to VOTable like in #2353. I found a solution that works 90% of the way to converting to VOTable but I ran into similar issues with the changes I made particularly for integral_rev3_scw testing. Maybe I'll just submit a PR for it to get some potential input, but have been at it in some free time for a few months now and haven't gotten anywhere.

Not sure if this is helpful or not, but just thought that since there was a mention on extending heasarc I would share a quick note on my experiences.

return patch_get

def test_custom_args(self):
object_name = 'Crab'
mission = 'intscw'
Expand Down
6 changes: 5 additions & 1 deletion astroquery/heasarc/tests/test_heasarc_remote_isdc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
@parametrization_local_save_remote
class TestHeasarcISDC:

@pytest.fixture(autouse=True)
def _patch_get(self, patch_get):
return patch_get

@property
def isdc_context(self):
return Conf.server.set_temp(
Expand Down Expand Up @@ -176,7 +180,7 @@ def test_mission_cols(self):
assert 'GOOD_ISGRI' in cols
assert 'RA_X' in cols
assert 'DEC_X' in cols
assert '_SEARCH_OFFSET' in cols
assert 'SEARCH_OFFSET_' in cols

def test_query_object_async(self):
mission = 'integral_rev3_scw'
Expand Down
Loading