DM-33394: Refactor the tests

[timj]

Checklist

• ran Jenkins
• added a release note for user-visible changes to doc/changes

[codecov]

Codecov Report

Merging #5 (9ebb5b0) into main (b524e60) will increase coverage by 4.23%.
The diff coverage is 94.66%.

@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
+ Coverage   85.46%   89.70%   +4.23%     
==========================================
  Files          15       21       +6     
  Lines        2071     2302     +231     
  Branches      297      329      +32     
==========================================
+ Hits         1770     2065     +295     
+ Misses        210      162      -48     
+ Partials       91       75      -16     

Impacted Files	Coverage Δ
python/lsst/resources/utils.py	62.26% <50.00%> (-3.05%)	⬇️
python/lsst/resources/http.py	69.62% <75.00%> (-0.52%)	⬇️
tests/test_s3.py	77.77% <77.77%> (ø)
tests/test_mem.py	82.35% <82.35%> (ø)
python/lsst/resources/file.py	88.88% <83.33%> (+15.23%)	⬆️
tests/test_resource.py	93.33% <93.33%> (ø)
tests/test_schemeless.py	93.33% <93.33%> (ø)
tests/test_http.py	95.91% <95.91%> (ø)
tests/test_file.py	96.70% <96.70%> (ø)
python/lsst/resources/tests.py	97.07% <97.07%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b524e60...9ebb5b0. Read the comment docs.

[rra]

If I understand this correctly, a lot of the changes are because the previous code assumed that if a ResourcePath was temporary, it was therefore safe to delete, but did not anticipate the case where a local temporary ResourcePath was itself passed to as_local and thus generated another layer of indirection that should not be allowed to delete itself because it's not the owner of the temporary. Assuming I have that right, this all looks good. That might be worth a general docstring somewhere because it's fairly inobvious.

Also, if I'm reading this correctly, the generic test cases are currently used only by files and S3, but the goal is to also use them for the WebDAV test cases eventually? (BTW, I would rename http to webdav in at least the name of the tests, since http makes me think it's some sort of generic http-backed resource, but the operations you're doing are very specific to WebDAV, which is a much more obscure protocol that most http stores won't support.)

[timj]

All the changes I did here were driven by test coverage uncovering problems. The fact that as_local could be given a temporary was one of those cases (previously as_local always returned that a local file was not temporary even if it was).

Yes, the hope was that schemes that support read/write would all just inherit from the standard base class TestCase. That works for file and s3 because moto acts like a mocked S3 backend that can remember state. responses does not do that at all and it seemed that to support it I'd have to write special responses code for each test -- I couldn't face doing that. Running up an actual test server seemed the only way forward although I have not tackled that here.

All the code switching is based entirely on URI scheme so calling it http.py seems right to me. The HTTP plugin can use normal URIs for read:

$ python -c 'from lsst.resources import ResourcePath; print(ResourcePath("https://raw.githubusercontent.com/lsst/daf_butler/main/python/lsst/daf/butler/configs/datastore.yaml").read().decode())'
/Users/timj/work/lsstsw3/miniconda/envs/lsst-scipipe-1.0.0/lib/python3.8/site-packages/urllib3/connectionpool.py:1043: InsecureRequestWarning: Unverified HTTPS request is being made to host 'raw.githubusercontent.com'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
  warnings.warn(
datastore:
  # Use file datastore as a default for a default butler
  cls: lsst.daf.butler.datastores.fileDatastore.FileDatastore

What other http-based URI standard would we use for uploading and moving files on a remote server?

[rra]

I agree with your analysis of the difficulty with testing WebDAV.

For whatever it's worth, I don't have any one association with an HTTP-based file manipulation protocol but I'm not sure I would think of WebDAV. S3 would probably be my first thought for something that manipulated files via HTTP, and beyond that I'd probably assume some custom REST thing. I think they all support download via simple HTTP. That said, it's certainly not a blocking comment or anything, just something that made me pause a moment because I wasn't expecting HTTP to actually mean WebDAV. It's a good point that WebDAV is probably the only protocol that uses the http scheme and verbs to do file manipulation on the server; maybe my intuition is just wrong here.

[timj]

It looks like webdav uses dav:// internally but expects users to always just use https://. At least for S3 and GCS there are standard schemes even if it's all https underneath.