Avoid assuming that access_url exists always
#570
Conversation
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.
There was a problem hiding this comment.
Until we have a more general solution for #543, I'd say this is a reasonable stop-gap measure.
The failing tests seem to be remote failures. I'd say this can live without a changelog entry, as the behaviour we are changing is neither sane nor expectable, nor will anyone rely on it.
I'll still look at #543 unless someone beats me to it (which I would like a lot!)
|
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. |
|
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)? |
|
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...)
|
|
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. |
|
@aragilar -- if you don't find time to fiddle a test let me know and I'll have a pass at it (I know these are a pain; I have tried to make making tests simpler and failed before). |
|
ping @aragilar about this, do you have an ETA for coming back to this PR? |
|
Sorry, been on leave for the past month, I'll try to get back into this soon (but if people want to jump in and modify things, feel free to do so). |
DatalinkResultsMixin was assuming that the records created by any subclass would have the
access_urlattribute. 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