Fix heasarc tests

[zoghbi-a]

This a fix for the heasarc tests. Specifically:

• Fix column name in test_heasarc_remote_isdc.test_mission_cols, so the test now passes.
• Move patch_get to class level to allow for other classes (in prep) that do not need it.

[pep8speaks]

Hello @zoghbi-a! 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 2023-10-23 10:49:10 UTC

[codecov]

Codecov Report

Merging #2842 (9c0350e) into main (37758c1) will decrease coverage by 0.03%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

❗ Current head 9c0350e differs from pull request most recent head c4f6ff4. Consider uploading reports for the commit c4f6ff4 to get more accurate results

@@            Coverage Diff             @@
##             main    #2842      +/-   ##
==========================================
- Coverage   66.51%   66.49%   -0.03%     
==========================================
  Files         235      235              
  Lines       18096    18081      -15     
==========================================
- Hits        12037    12023      -14     
+ Misses       6059     6058       -1     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bsipocz]

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)

[zoghbi-a]

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?

[zoghbi-a]

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.

[bsipocz]

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

[bsipocz]

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.

[zoghbi-a]

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%]

[bsipocz]

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.

[zoghbi-a]

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?

[bsipocz]

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) ============================================

[nkphysics]

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.

[zoghbi-a]

@bsipocz,
Do you think we can merge this so the tests work? I will submit a separate PR for the class changes.

[bsipocz]

The column name back-and-forth needs to be fixed by updating the name to pass the remote queries and thus the canned data file in the mock tests also needs the column name fixed.

fixture fixes look good, and I agree that reorganizing tests can be done in a follow-up.

[bsipocz]

There has been a bit of a back-and-forth with this column name. Could you consolidate those changes to actually fix rather than skip the test? E.g. have a pass with the remote-data tests, and update the canned data file accordingly to get the mocked tests pass.

[zoghbi-a]

I don't know how to update the data files. I tried deleting them and running remote tests, but they didn't update

[bsipocz]

you need to either manually edit, or manually overwrite them with a freshly downloaded version (as ideally the datafiles we have canned up in the test suite are actually relevant versions of the remote responses).

[zoghbi-a]

The remaining failure is from the cadc tests.

[bsipocz]

Thank you so much @zoghbi-a!