Merge pull request #2526 from jdavies-st/2107-test-files-left-behind

MNT: Fix tests that leave behind files in the repo

Author: bsipocz

.github/workflows/ci_crontests.yml

@@ -8,6 +8,7 @@ on:
   schedule:
     # run every Monday at 5am UTC
     - cron: '0 5 * * 1'
+  workflow_dispatch:
 
 permissions:
   contents: read

astroquery/alma/tests/test_alma_remote.py

@@ -1,17 +1,15 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
-import tempfile
-import shutil
-import numpy as np
-import pytest
-
 from datetime import datetime
 import os
+from pathlib import Path
 from urllib.parse import urlparse
 import re
 from unittest.mock import Mock, MagicMock, patch
 
 from astropy import coordinates
 from astropy import units as u
+import numpy as np
+import pytest
 
 from astroquery.exceptions import CorruptDataWarning
 from astroquery.utils.commons import ASTROPY_LT_4_1
@@ -49,15 +47,6 @@ def alma(request):
 
 @pytest.mark.remote_data
 class TestAlma:
-    @pytest.fixture()
-    def temp_dir(self, request):
-        my_temp_dir = tempfile.mkdtemp()
-
-        def fin():
-            shutil.rmtree(my_temp_dir)
-        request.addfinalizer(fin)
-        return my_temp_dir
-
     def test_public(self, alma):
         results = alma.query(payload=None, public=True, maxrec=100)
         assert len(results) == 100
@@ -68,8 +57,8 @@ def test_public(self, alma):
         for row in results:
             assert row['data_rights'] == 'Proprietary'
 
-    def test_SgrAstar(self, temp_dir, alma):
-        alma.cache_location = temp_dir
+    def test_SgrAstar(self, tmp_path, alma):
+        alma.cache_location = tmp_path
 
         result_s = alma.query_object('Sgr A*', legacy_columns=True)
 
@@ -122,9 +111,9 @@ def test_ra_dec(self, alma):
         assert len(result) > 0
 
     @pytest.mark.skipif("SKIP_SLOW")
-    def test_m83(self, temp_dir, alma):
+    def test_m83(self, tmp_path, alma):
         # Runs for over 9 minutes
-        alma.cache_location = temp_dir
+        alma.cache_location = tmp_path
 
         m83_data = alma.query_object('M83', science=True, legacy_columns=True)
         uids = np.unique(m83_data['Member ous id'])
@@ -149,20 +138,20 @@ def test_data_proprietary(self, alma):
         with pytest.raises(AttributeError):
             alma.is_proprietary('uid://NON/EXI/STING')
 
-    def test_retrieve_data(self, temp_path, alma):
+    def test_retrieve_data(self, tmp_path, alma):
         """
         Regression test for issue 2490 (the retrieval step will simply fail if
         given a blank line, so all we're doing is testing that it runs)
         """
-        alma.cache_location = temp_path
+        alma.cache_location = tmp_path
 
         # small solar TP-only data set (<1 GB)
         uid = 'uid://A001/X87c/X572'
 
         alma.retrieve_data_from_uid([uid])
 
-    def test_data_info(self, temp_dir, alma):
-        alma.cache_location = temp_dir
+    def test_data_info(self, tmp_path, alma):
+        alma.cache_location = tmp_path
 
         uid = 'uid://A001/X12a3/Xe9'
         data_info = alma.get_data_info(uid, expand_tarfiles=True)
@@ -189,8 +178,8 @@ def test_data_info(self, temp_dir, alma):
                 file_url = url
                 break
         assert file_url
-        alma.download_files([file_url], savedir=temp_dir)
-        assert os.stat(os.path.join(temp_dir, file)).st_size
+        alma.download_files([file_url], savedir=tmp_path)
+        assert Path(tmp_path, file).stat().st_size
 
         # mock downloading an entire program
         download_files_mock = Mock()
@@ -200,9 +189,9 @@ def test_data_info(self, temp_dir, alma):
         comparison = download_files_mock.mock_calls[0][1] == data_info_tar['access_url']
         assert comparison.all()
 
-    def test_download_data(self, temp_dir, alma):
+    def test_download_data(self, tmp_path, alma):
         # test only fits files from a program
-        alma.cache_location = temp_dir
+        alma.cache_location = tmp_path
 
         uid = 'uid://A001/X12a3/Xe9'
         data_info = alma.get_data_info(uid, expand_tarfiles=True)
@@ -214,14 +203,14 @@ def test_download_data(self, temp_dir, alma):
         alma._download_file = download_mock
         urls = [x['access_url'] for x in data_info
                 if fitsre.match(x['access_url'])]
-        results = alma.download_files(urls, savedir=temp_dir)
+        results = alma.download_files(urls, savedir=tmp_path)
         alma._download_file.call_count == len(results)
         assert len(results) == len(urls)
 
-    def test_download_and_extract(self, temp_dir, alma):
+    def test_download_and_extract(self, tmp_path, alma):
         # TODO: slowish, runs for ~90s
 
-        alma.cache_location = temp_dir
+        alma.cache_location = tmp_path
         alma._cycle0_tarfile_content_table = {'ID': ''}
 
         uid = 'uid://A001/X12a3/Xe9'
@@ -261,10 +250,10 @@ def test_download_and_extract(self, temp_dir, alma):
                     [asdm_url], include_asdm=True, regex=r'.*\.py')
         delete_mock.assert_called_once_with(
             'cache_path/' + asdm_url.split('/')[-1])
-        assert downloaded_asdm == [os.path.join(temp_dir, 'foo.py')]
+        assert downloaded_asdm == [Path(tmp_path, 'foo.py')]
 
-    def test_doc_example(self, temp_dir, alma):
-        alma.cache_location = temp_dir
+    def test_doc_example(self, tmp_path, alma):
+        alma.cache_location = tmp_path
         m83_data = alma.query_object('M83', legacy_columns=True)
         # the order can apparently sometimes change
         # These column names change too often to keep testing.
@@ -299,8 +288,8 @@ def test_doc_example(self, temp_dir, alma):
         # file sizes are replaced with -1
         assert (totalsize_mous.to(u.GB).value > 52)
 
-    def test_query(self, temp_dir, alma):
-        alma.cache_location = temp_dir
+    def test_query(self, tmp_path, alma):
+        alma.cache_location = tmp_path
 
         result = alma.query(payload={'start_date': '<11-11-2011'},
                             public=False, legacy_columns=True, science=True)
@@ -395,9 +384,9 @@ def test_user(self, alma):
     # This has been reported, as it is definitely a bug.
     @pytest.mark.xfail
     @pytest.mark.bigdata
-    def test_cycle1(self, temp_dir, alma):
+    def test_cycle1(self, tmp_path, alma):
         # About 500 MB
-        alma.cache_location = temp_dir
+        alma.cache_location = tmp_path
         target = 'NGC4945'
         project_code = '2012.1.00912.S'
         payload = {'project_code': project_code,
@@ -442,9 +431,9 @@ def test_cycle1(self, temp_dir, alma):
 
     @pytest.mark.skipif("SKIP_SLOW")
     @pytest.mark.xfail(reason="Not working anymore")
-    def test_cycle0(self, temp_dir, alma):
+    def test_cycle0(self, tmp_path, alma):
         # About 20 MB
-        alma.cache_location = temp_dir
+        alma.cache_location = tmp_path
 
         target = 'NGC4945'
         project_code = '2011.0.00121.S'
@@ -477,7 +466,7 @@ def test_cycle0(self, temp_dir, alma):
         # There are 10 small files, but only 8 unique
         assert len(data) == 8
 
-    def test_keywords(self, temp_dir, alma):
+    def test_keywords(self, tmp_path, alma):
 
         alma.help_tap()
         result = alma.query_tap(
@@ -545,44 +534,34 @@ def test_big_download_regression(alma):
 
 
 @pytest.mark.remote_data
-def test_download_html_file(alma):
+def test_download_html_file(alma, tmp_path):
+    alma.cache_location = tmp_path
     result = alma.download_files(['https://{}/dataPortal/member.uid___A001_X1284_X1353.qa2_report.html'.format(download_hostname)])
     assert result
 
 
 @pytest.mark.remote_data
-def test_verify_html_file(alma, caplog):
-    # first, make sure the file is not cached (in case this test gets called repeatedly)
-    # (we are hacking the file later in this test to trigger different failure modes so
-    # we need it fresh)
-    try:
-        result = alma.download_files(['https://{}/dataPortal/member.uid___A001_X1284_X1353.qa2_report.html'.format(download_hostname)], verify_only=True)
-        local_filepath = result[0]
-        os.remove(local_filepath)
-    except FileNotFoundError:
-        pass
-
-    caplog.clear()
+def test_verify_html_file(alma, caplog, tmp_path):
+    alma.cache_location = tmp_path
 
     # download the file
     result = alma.download_files(['https://{}/dataPortal/member.uid___A001_X1284_X1353.qa2_report.html'.format(download_hostname)])
     assert 'member.uid___A001_X1284_X1353.qa2_report.html' in result[0]
 
     result = alma.download_files(['https://{}/dataPortal/member.uid___A001_X1284_X1353.qa2_report.html'.format(download_hostname)], verify_only=True)
     assert 'member.uid___A001_X1284_X1353.qa2_report.html' in result[0]
-    local_filepath = result[0]
-    existing_file_length = 66336
-    assert f"Found cached file {local_filepath} with expected size {existing_file_length}." in caplog.text
+    local_filepath = Path(result[0])
+    expected_file_length = local_filepath.stat().st_size
+    assert f"Found cached file {local_filepath} with expected size {expected_file_length}." in caplog.text
 
     # manipulate the file
     with open(local_filepath, 'ab') as fh:
         fh.write(b"Extra Text")
 
     caplog.clear()
-    length = 66336
-    existing_file_length = length + 10
+    new_file_length = expected_file_length + 10
     with pytest.warns(expected_warning=CorruptDataWarning,
-            match=f"Found cached file {local_filepath} with size {existing_file_length} > expected size {length}.  The download is likely corrupted."):
+                      match=f"Found cached file {local_filepath} with size {new_file_length} > expected size {expected_file_length}.  The download is likely corrupted."):
         result = alma.download_files(['https://{}/dataPortal/member.uid___A001_X1284_X1353.qa2_report.html'.format(download_hostname)], verify_only=True)
     assert 'member.uid___A001_X1284_X1353.qa2_report.html' in result[0]
 
@@ -593,10 +572,5 @@ def test_verify_html_file(alma, caplog):
     caplog.clear()
     result = alma.download_files(['https://{}/dataPortal/member.uid___A001_X1284_X1353.qa2_report.html'.format(download_hostname)], verify_only=True)
     assert 'member.uid___A001_X1284_X1353.qa2_report.html' in result[0]
-    length = 66336
     existing_file_length = 10
-    assert f"Found cached file {local_filepath} with size {existing_file_length} < expected size {length}.  The download should be continued." in caplog.text
-
-    # cleanup: we don't want `test_download_html_file` to fail if this test is re-run
-    if os.path.exists(local_filepath):
-        os.remove(local_filepath)
+    assert f"Found cached file {local_filepath} with size {existing_file_length} < expected size {expected_file_length}.  The download should be continued." in caplog.text

astroquery/cds/core.py

@@ -256,7 +256,7 @@ def _args_to_payload(self, **kwargs):
                 self.path_moc_file = os.path.join(os.getcwd(), 'moc.fits')
                 if os.path.isfile(self.path_moc_file):  # Silent overwrite
                     os.remove(self.path_moc_file)
-                region.write(self.path_moc_file, format="fits")
+                region.save(self.path_moc_file, format="fits")
                 # add the moc region payload to the request payload
             elif isinstance(region, CircleSkyRegion):
                 # add the cone region payload to the request payload

astroquery/cds/tests/test_mocserver_remote.py

@@ -68,7 +68,7 @@ def test_field_l_param(self, field_l):
     @pytest.mark.skipif('mocpy' not in sys.modules,
                         reason="requires MOCPy")
     @pytest.mark.parametrize('moc_order', [5, 10])
-    def test_moc_order_param(self, moc_order):
+    def test_moc_order_param(self, moc_order, tmp_cwd):
         moc_region = MOC.from_json({'0': [1]})
 
         result = cds.query_region(region=moc_region,

astroquery/conftest.py

@@ -1,5 +1,8 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
 import os
+from pathlib import Path
+
+import pytest
 # this contains imports plugins that configure py.test for astropy tests.
 # by importing them here in conftest.py they are discoverable by py.test
 # no matter how it is invoked within the source tree.
@@ -57,3 +60,14 @@ def pytest_addoption(parser):
         help='ALMA site (almascience.nrao.edu, almascience.eso.org or '
              'almascience.nao.ac.jp for example)'
     )
+
+
+@pytest.fixture(scope='function')
+def tmp_cwd(tmp_path):
+    """Perform test in a pristine temporary working directory."""
+    old_dir = Path.cwd()
+    os.chdir(tmp_path)
+    try:
+        yield tmp_path
+    finally:
+        os.chdir(old_dir)

astroquery/esa/iso/tests/test_iso.py

@@ -58,7 +58,7 @@ def test_get_postcard_link_verbose(self):
         link = ida.get_postcard_link(**parameters)
 
     @pytest.mark.remote_data
-    def test_download_data(self):
+    def test_download_data(self, tmp_cwd):
         parameters = {'tdt': "40001501",
                       'product_level': "DEFAULT_DATA_SET",
                       'retrieval_type': "OBSERVATION",
@@ -69,7 +69,7 @@ def test_download_data(self):
         assert res == "file.tar", "File name is " + res + " and not file.tar"
 
     @pytest.mark.remote_data
-    def test_download_postcard(self):
+    def test_download_postcard(self, tmp_cwd):
         parameters = {'tdt': "40001501",
                       'filename': "file",
                       'verbose': False}

astroquery/esa/xmm_newton/tests/test_xmm_newton_remote.py

@@ -112,7 +112,7 @@ def _create_tar(self, tarname, files):
                     shutil.rmtree(ob_name)
 
     @pytest.mark.remote_data
-    def test_download_data(self):
+    def test_download_data(self, tmp_cwd):
         parameters = {'observation_id': "0112880801",
                       'level': "ODF",
                       'filename': 'file',
@@ -121,7 +121,7 @@ def test_download_data(self):
         xsa.download_data(**parameters)
 
     @pytest.mark.remote_data
-    def test_download_data_single_file(self):
+    def test_download_data_single_file(self, tmp_cwd):
         parameters = {'observation_id': "0762470101",
                       'level': "PPS",
                       'name': 'OBSMLI',
@@ -133,7 +133,7 @@ def test_download_data_single_file(self):
         xsa.download_data(**parameters)
 
     @pytest.mark.remote_data
-    def test_get_postcard(self):
+    def test_get_postcard(self, tmp_cwd):
         parameters = {'observation_id': "0112880801",
                       'image_type': "OBS_EPIC",
                       'filename': None,
@@ -142,7 +142,7 @@ def test_get_postcard(self):
         xsa.get_postcard(**parameters)
 
     @pytest.mark.remote_data
-    def test_get_postcard_filename(self):
+    def test_get_postcard_filename(self, tmp_cwd):
         parameters = {'observation_id': "0112880801",
                       'image_type': "OBS_EPIC",
                       'filename': "test",
@@ -195,7 +195,6 @@ def test_get_epic_metadata(self):
         assert report_diff_values(slew_source, table)
 
     @pytest.mark.remote_data
-    @pytest.mark.xfail(raises=LoginError)
     def test_download_proprietary_data_incorrect_credentials(self):
         parameters = {'observation_id': "0762470101",
                       'prop': 'True',
@@ -207,10 +206,10 @@ def test_download_proprietary_data_incorrect_credentials(self):
                       'extension': 'FTZ',
                       'verbose': False}
         xsa = XMMNewtonClass(self.get_dummy_tap_handler())
-        xsa.download_data(**parameters)
+        with pytest.raises(LoginError):
+            xsa.download_data(**parameters)
 
     @pytest.mark.remote_data
-    @pytest.mark.xfail(raises=LoginError)
     def test_download_proprietary_data_without_credentials(self):
         parameters = {'observation_id': "0883780101",
                       'level': "PPS",
@@ -220,10 +219,11 @@ def test_download_proprietary_data_without_credentials(self):
                       'extension': 'FTZ',
                       'verbose': False}
         xsa = XMMNewtonClass(self.get_dummy_tap_handler())
-        xsa.download_data(**parameters)
+        with pytest.raises(LoginError):
+            xsa.download_data(**parameters)
 
     @pytest.mark.remote_data
-    def test_get_epic_spectra(self):
+    def test_get_epic_spectra(self, tmp_cwd):
         _tarname = "tarfile.tar"
         _source_number = 83
         _instruments = ["M1", "M1_arf", "M1_bkg", "M1_rmf",