Escape MAST Download URIs #3080
Conversation
| @@ -150,10 +150,6 @@ def test_mast_service_request_async(self): | |||
| assert isinstance(responses, list) | |||
|
|
|||
| def test_mast_service_request(self): | |||
|
|
|||
| # clear columns config | |||
| Mast._column_configs = dict() | |||
There was a problem hiding this comment.
it looks like this and the other assignments to dict() were done to reset the class attribute _column_configs. Are we sure we don't need this anymore?
There was a problem hiding this comment.
I'm not 100% sure what purpose the lines serve, but removing them has no impact on the test results. From what I can tell, _column_configs seems to be a sort of caching variable to avoid having to make multiple calls for the columnsConfig entry for a service. I don't see any apparent reason that this should be reset for certain tests, since I don't imagine that we expect the configuration for a service to change all that often. What do you think?
|
Would it make sense to add a test here with a URI that requires the escaped behavior? Something like: |
|
Added a test! Thank you for that URI example, it's a pretty quick download. |
| assert Path(tmp_path, filename).exists() | ||
|
|
||
| # check that downloaded file is a valid FITS file | ||
| f = fits.open(filename) |
There was a problem hiding this comment.
Need to include the path to open the file here. This passes in CI because remote tests aren't run, but I ran it locally.
| f = fits.open(filename) | |
| f = fits.open(Path(tmp_path, filename)) |
FWIW, adding +1 min to the remote test runtime is not a big deal, there are bigger problems in that ~2hr long run that we can fix first. |
Fixes an issue with downloading URIs that include URL parameters. This PR escapes URIs before sending them to the MAST download service so that URIs are not unexpectedly truncated.
Also removes lines from the test suite that were not serving an apparent purpose.
Adding a test case for this particular change would add ~1 minute to test execution time, so I was planning to add that to our internal, backend test suite.