search

astropy / pyvo

An Astropy affiliated package providing access to remote data and services of the Virtual Observatory (VO) using Python.
https://pyvo.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
74 stars 50 forks source link

Avoid assuming that `access_url` exists always #570

Open aragilar opened 1 week ago

aragilar commented 1 week ago

DatalinkResultsMixin was assuming that the records created by any subclass would have the access_url attribute. This is not true for SSA (whether it should do is a separate question). This replaces that assumption with a check for existence first, and then if that fails, fall back to getdatalink on the record.

See #569 for more context.

I'm not sure this is the right solution (especially around the fallback, as that's what the previous code did before the batching in #218), but it does work, both in that I no longer get an exception, and I can iterate over the datalink urls in the SSA results.

Fixes: #569

aragilar commented 1 week ago

Oh, I missed #543, as I was looking for SSA-related issues. As I noted, I'm not clear on which classes/mixins are supposed to be responsible for which parts, and what subclasses are expected to implement.

aragilar commented 1 week ago

Sure, should I dump an example VOTable in the data dir and try to load it? Do we have a expected way to mock out requests (as at least in my case, the service in still at the beta url, and so the urls aren't stable yet)?

msdemlei commented 1 week ago

On Thu, Jul 04, 2024 at 07:48:54PM -0700, James Tocknell wrote:

Sure, should I dump an example VOTable in the data dir and try to load it? Do we have a expected way to mock out requests (as at least in my case, the service in still at the beta url, and so the urls aren't stable yet)?

Yes, please avoid remote data tests when possible in order to keep the number of boxes we depend on for our tests low.

We're using requests_mock; registry/tests/test_regtap.py has several examples for how to do this (e.g., the capabilities fixture).

I've always wanted to write some docs for this (and then think about whether the way we do this is actually the simplest way there is...)

bsipocz commented 3 days ago

There are some minimal test mocking guidelines at astroquery, it would be nice to make one that works for both packages: https://astroquery.readthedocs.io/en/latest/testing.html

But overall, I agree with Markus' suggestion, your best bet is to look for existing tests that do similar mocking and copy those as we don't really have good enough docs for this.