siphon icon indicating copy to clipboard operation
siphon copied to clipboard

Bug Fix: Populate Dataset access_urls

Open jamespolly opened this issue 2 years ago • 7 comments

Description Of Changes

This is an incremental PR that will need some attention to testing and additional input. Intended to get the ball rolling.

Minor change to catalog.py which permits populating Dataset.access_urls in situations where service_name is not in all_service_dict.

See #734 and #715.

38 tests were failing prior to my change. 41 tests failed afterward. Common themes across the three new failures:

> FAILED tests/test_catalog.py::test_simple_service_within_compound - AssertionError: assert {'OPENDAP': '...20170824.txt'} == {'HTTPServer'...20170824.txt'}
22a24,25
> FAILED tests/test_catalog_access.py::test_case_insensitive_access - AssertionError: assert 'OPENDAP' == 'HTTPSERVER'
> FAILED tests/test_catalog_access.py::test_manage_access_types_case_insensitive - AssertionError: assert {'OPENDAP': '...20170824.txt'} == {'HTTPSERVER'...20170824.txt'}

OPeNDAP is chosen as a first (and sometimes only) access_url rather than HTTPServer. Oddly I cannot replicate these assertion errors in ipython.

Checklist

  • [x] Closes #734 and #715
  • [x] Tests added but see above for fixes needed to existing tests.
  • [x] Fully documented

jamespolly avatar Sep 22 '23 14:09 jamespolly

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Sep 22 '23 14:09 CLAassistant

Taking another stab to get the tests passing in the CI. My opening comment in this PR mentioned 38 test failing on my machine when following the test instructions. Only three tests failed in the CI so there's clearly a difference in these testing implementations.

In any event trying to get those failed tests passing so this can be reviewed.

jamespolly avatar Oct 10 '23 19:10 jamespolly

@dopplershift Can I rerun the CI here or is that beyond my permissions?

jamespolly avatar Oct 11 '23 19:10 jamespolly

Just kicked them off, thanks for the poke.

dopplershift avatar Oct 11 '23 19:10 dopplershift

@dopplershift Well this was embarrassing. Once more time would be appreciated.

jamespolly avatar Oct 11 '23 20:10 jamespolly

Thank you @dopplershift. Looks like flake8 and tests are passing. There are some other checks not passing (Docs Build) but I don't think I changed anything here.

Please let me know if that is something I should fix!

jamespolly avatar Oct 11 '23 21:10 jamespolly

@dopplershift Looks like there are some lower-level build failures (numpy and cartopy) that are happening in the CI. Have we seen these before? Seems unrelated to the changes made in this PR. Re-poking just incase.

jamespolly avatar Oct 19 '23 19:10 jamespolly