Avoid assuming that `access_url` exists always

[aragilar]

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]

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]

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]

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]

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.

[msdemlei]

@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).

[bsipocz]

ping @aragilar about this, do you have an ETA for coming back to this PR?

[aragilar]

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).

[msdemlei]

Ahwait. This code will dramatically change with PR #599. There's really not much point adding tests for it now. So, I will go ahead and merge the branch so there is a record in the history of the immediate bug fix (@aragilar, I'd hope #599 fixes your bug, too; would you close your bug if it does and #599 is merged?) and then rebase #599, which will replace this code anyway.

Sorry I have not noticed this earlier.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.43%. Comparing base (13e4abe) to head (afce8e5). Report is 126 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/dal/adhoc.py	0.00%	6 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
- Coverage   81.47%   81.43%   -0.04%     
==========================================
  Files          68       68              
  Lines        7039     7042       +3     
==========================================
  Hits         5735     5735              
- Misses       1304     1307       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.