Merge pull request #2224 from andamian/almafix

ALMA integration tests fix

Author: bsipocz

CHANGES.rst

@@ -16,6 +16,8 @@ sdss
 Infrastructure, Utility and Other Changes and Additions
 -------------------------------------------------------
 
+- Adding ``--alma-site`` pytest option for testing to have a control over
+  which specific site to test. [#2224]
 
 
 0.4.4 (2021-11-17)

astroquery/alma/__init__.py

@@ -11,8 +11,6 @@
              'https://almascience.nrao.edu',
              'https://almascience.nao.ac.jp']
 
-_test_url_list = ['https://beta.cadc-ccda.hia-ha.nrc-cnrc.gc.ca']
-
 auth_urls = ['asa.alma.cl', 'rh-cas.alma.cl']
 
 
@@ -27,11 +25,6 @@ class Conf(_config.ConfigNamespace):
         _url_list,
         'The ALMA Archive mirror to use.')
 
-    test_archive_url = _config.ConfigItem(
-        _test_url_list,
-        'ALMA Archive Test Mirrors (temporary)'
-    )
-
     auth_url = _config.ConfigItem(
         auth_urls,
         'ALMA Central Authentication Service URLs'

astroquery/alma/core.py

@@ -110,7 +110,7 @@
     },
     'Energy': {
         'Frequency (GHz)': ['frequency', 'frequency', _gen_numeric_sql],
-        'Bandwidth (GHz)': ['bandwidth', 'bandwidth', _gen_numeric_sql],
+        'Bandwidth (Hz)': ['bandwidth', 'bandwidth', _gen_numeric_sql],
         'Spectral resolution (KHz)': ['spectral_resolution',
                                       'em_resolution', _gen_spec_res_sql],
         'Band': ['band_list', 'band_list', _gen_band_list_sql]
@@ -677,7 +677,7 @@ def _HEADER_data_size(self, files):
         return data_sizes, totalsize.to(u.GB)
 
     def download_files(self, files, savedir=None, cache=True,
-                       continuation=True, skip_unauthorized=True):
+                       continuation=True, skip_unauthorized=True,):
         """
         Given a list of file URLs, download them
 
@@ -708,22 +708,21 @@ def download_files(self, files, savedir=None, cache=True,
         downloaded_files = []
         if savedir is None:
             savedir = self.cache_location
-        for fileLink in unique(files):
-            log.debug("Downloading {0} to {1}".format(fileLink, savedir))
+        for file_link in unique(files):
+            log.debug("Downloading {0} to {1}".format(file_link, savedir))
             try:
-                check_filename = self._request('HEAD', fileLink, auth=auth,
-                                               stream=True)
+                check_filename = self._request('HEAD', file_link, auth=auth)
                 check_filename.raise_for_status()
             except requests.HTTPError as ex:
                 if ex.response.status_code == 401:
                     if skip_unauthorized:
                         log.info("Access denied to {url}.  Skipping to"
-                                 " next file".format(url=fileLink))
+                                 " next file".format(url=file_link))
                         continue
                     else:
                         raise(ex)
 
-            if 'text/html' in check_filename.headers['Content-Type']:
+            if 'text/html' in check_filename.headers.get('Content-Type', ''):
                 raise ValueError("Bad query.  This can happen if you "
                                  "attempt to download proprietary "
                                  "data when not logged in")
@@ -732,7 +731,7 @@ def download_files(self, files, savedir=None, cache=True,
                 filename = re.search("filename=(.*)",
                                      check_filename.headers['Content-Disposition']).groups()[0]
             except KeyError:
-                log.info(f"Unable to find filename for {fileLink}  "
+                log.info(f"Unable to find filename for {file_link}  "
                          "(missing Content-Disposition in header).  "
                          "Skipping to next file.")
                 continue
@@ -742,42 +741,42 @@ def download_files(self, files, savedir=None, cache=True,
                                         filename)
 
             try:
-                self._download_file(fileLink,
+                self._download_file(file_link,
                                     filename,
                                     timeout=self.TIMEOUT,
                                     auth=auth,
                                     cache=cache,
                                     method='GET',
-                                    head_safe=True,
+                                    head_safe=False,
                                     continuation=continuation)
 
                 downloaded_files.append(filename)
             except requests.HTTPError as ex:
                 if ex.response.status_code == 401:
                     if skip_unauthorized:
                         log.info("Access denied to {url}.  Skipping to"
-                                 " next file".format(url=fileLink))
+                                 " next file".format(url=file_link))
                         continue
                     else:
                         raise(ex)
                 elif ex.response.status_code == 403:
-                    log.error("Access denied to {url}".format(url=fileLink))
-                    if 'dataPortal' in fileLink and 'sso' not in fileLink:
+                    log.error("Access denied to {url}".format(url=file_link))
+                    if 'dataPortal' in file_link and 'sso' not in file_link:
                         log.error("The URL may be incorrect.  Try using "
                                   "{0} instead of {1}"
-                                  .format(fileLink.replace('dataPortal/',
-                                                           'dataPortal/sso/'),
-                                          fileLink))
+                                  .format(file_link.replace('dataPortal/',
+                                                            'dataPortal/sso/'),
+                                          file_link))
                     raise ex
                 elif ex.response.status_code == 500:
                     # empirically, this works the second time most of the time...
-                    self._download_file(fileLink,
+                    self._download_file(file_link,
                                         filename,
                                         timeout=self.TIMEOUT,
                                         auth=auth,
                                         cache=cache,
                                         method='GET',
-                                        head_safe=True,
+                                        head_safe=False,
                                         continuation=continuation)
 
                     downloaded_files.append(filename)
@@ -1107,7 +1106,7 @@ def download_and_extract_files(self, urls, delete=True, regex=r'.*\.fits$',
                 expanded_files += [x for x in files['access_url'] if
                                    filere.match(x.split('/')[-1])]
             else:
-                tar_files.append(tar_file)
+                tar_files.append(url)
 
         try:
             # get the tar files

astroquery/alma/tests/test_alma.py

@@ -410,3 +410,34 @@ def test_galactic_query():
                                radius=1*u.deg, get_query_payload=True)
 
     assert result['ra_dec'] == SkyCoord(0*u.deg, 0*u.deg, frame='galactic').icrs.to_string() + ", 1.0"
+
+
+def test_download_files():
+    def _requests_mock(method, url, **kwargs):
+        response = Mock()
+        response.headers = {
+            'Content-Disposition': 'attachment; '
+                                   'filename={}'.format(url.split('/')[-1])}
+        return response
+
+    def _download_file_mock(url, file_name, **kwargs):
+        return file_name
+    alma = Alma()
+    alma._request = Mock(side_effect=_requests_mock)
+    alma._download_file = Mock(side_effect=_download_file_mock)
+    downloaded_files = alma.download_files(['https://location/file1'])
+    assert len(downloaded_files) == 1
+    assert downloaded_files[0].endswith('file1')
+
+    alma._request.reset_mock()
+    alma._download_file.reset_mock()
+    downloaded_files = alma.download_files(['https://location/file1',
+                                            'https://location/file2'])
+    assert len(downloaded_files) == 2
+
+    # error cases
+    alma._request = Mock()
+    # no Content-Disposition results in no downloaded file
+    alma._request.return_value = Mock(headers={})
+    result = alma.download_files(['https://location/file1'])
+    assert not result