DM-37917: Add testing against real webDAV server for HttpResourcePath #40

airnandez · 2023-02-08T14:27:21Z

Checklist

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

…esting

This makes it consistent with the other interfaces.

timj

@airnandez thanks for adding these tests. I took the liberty of trying to enable the full ResourcePath test suite now that we have a functioning WebDAV server and not a mocked one. It didn't take many lines to enable it and it did find two compatibility issues.

When a directory is not created because it's not a directory we have to raise NotADirectoryError (I fixed this).
When attempting to uri.write() when the parent directory does not exist, the other backends make the directory automatically. This does not happen in webdav and so the 4 tests fail with error 409.

Now that we have a WebDAV server involved in the tests I wonder if the mocked test case is worth keeping now. Its main reason to exist is that it tries to test code when there is no webdav server running so maybe that's enough of a reason to keep it but it's now a lot of code to continue to support and it's also not that easy to follow.

* Run generic Resource tests in addition to HTTP-specific tests * Skip tests again plain HTTP servers because they cannot succeed since methods such as mkdir(), exists() for a directory or remove() for a directory are not supported by those servers. * Allow to run the same set of tests against real servers in the development environment by allowing to specify the server URL via an environment variable. * Skip mock-based tests for now that we can run against a real local server launched at test time.

airnandez · 2023-02-10T16:09:51Z

... uri.write() when the parent directory does not exist, the other backends make the directory automatically.

This is fixed now.

Now that we have a WebDAV server involved in the tests I wonder if the mocked test case is worth keeping now.

I agree that mocking responses is tedious and not very reliable. For the time being, I skip that test as well as all the generic tests that need HttpResourcePath.walk() to be implemented, which is not yet the case.

Also, let me draw your attention to the fact that it is not possible to run the generic tests against a plain HTTP backend, since several methods like mkdir(), remove() for directories, exists() for directories cannot be implemented.

Finally, I ran the same set of generic tests and the webDAV-specific tests against 4 different live DAV servers in my development environment.

timj

Looks good. Some minor cleanups suggested (especially removing code completely that we are always skipping).

timj · 2023-02-10T17:56:10Z

tests/test_http.py

+# walk() and we can fully test that GenericTestCase passes against a
+# real webDAV server. For the time being we skip it because mocking
+# all the possible responses for every possible situation is tedious.
+@unittest.skipIf(True, "Skipping test with mocked responses.")


I'd rather we deleted the test case completely if we have no intention of keeping it.

Agreed. Tests with mocked responses removed.

timj · 2023-02-10T18:01:17Z

tests/test_http.py

+# Execution of GenericTestCase against a plain HTTP server cannot succeed:
+# such a server does not support mkdir(), exists() for a directory, remove()
+# for a directory.
+@unittest.skipIf(True, "Skipping testing with plain HTTP server.")


Why are we skipping this? The GenericTestCase code works fine and is there to ensure that the path operations function properly for the HttpResourcePath -- it's specifically designed not to do any I/O and all tests pass when enabled. Please remove the skip and comment.

Wrong understanding on my side. I reactivated this test.

timj · 2023-02-10T18:21:23Z

tests/test_http.py

+            # developer environment by initializing the environment variable
+            # with the URL of the server, e.g.
+            #    https://dav.example.org:1234/path/to/top/dir
+            u = urllib.parse.urlparse(test_endpoint)


Since this is in the ResourcePath codebase can we use it rather than the explicit parsing?

test_uri = ResourcePath(test_endpoint, forceDirectory=True) cls.scheme = test_uri.scheme cls.netloc = test_uri.netloc cls.base_path = test_uri.path

?

The intention was to get a clean path, to avoid potential issues with super-sensitive webDAV servers:

>>> p = ResourcePath("https://host.example.org:12345////not/very////clean/path///") >>> p.path '////not/very////clean/path///'

By a "clean path" I mean a path in which each component is separated by a single /, e.g.:

/not/very/clean/path

A path with several separators is syntactically correct, though.

I reverted to use ResourcePath for parsing.

timj · 2023-02-10T18:22:09Z

tests/test_http.py

+            u = urllib.parse.urlparse(test_endpoint)
+            cls.scheme = u.scheme
+            cls.netloc = u.netloc
+            cls.base_path = "/" + u.path.strip("/")


Why are we stripping the leading "/" and then adding it back again? Why are we stripping the trailing slash when the code that uses it takes that into account?

See comment above.

timj · 2023-02-10T18:25:34Z

python/lsst/resources/tests.py

+
+            path = path.lstrip("/")
+            if self.base_path is not None:
+                path = self.base_path.strip("/") + "/" + path


Probably clearer to do:

path = os.path.join(self.base_path, path)

since that will take care of the trailing slash automatically.

timj · 2023-02-10T18:25:56Z

python/lsst/resources/tests.py

+
+            path = path.lstrip("/")
+            if self.base_path is not None:
+                path = self.base_path.strip("/") + "/" + path
            if path.startswith("/"):


The path.lstrip at the top makes this line a no-op I think.

See comment above.

timj · 2023-02-10T18:48:41Z

python/lsst/resources/tests.py

@@ -428,7 +436,10 @@ def setUp(self) -> None:
            self.tmpdir = ResourcePath(makeTestTempDir(self.testdir))
        else:
            # Create tmp directory relative to the test root.


Maybe change to "Create random tmp directory"

timj · 2023-02-10T19:13:54Z

tests/test_http.py

+            shutil.rmtree(cls.webdav_tmpdir, ignore_errors=True)
+
+    def setUp(self):
+        super().setUp()


I think a method that just calls super() can be deleted entirely.

…environment variable is defined

…n of status code

codecov · 2023-02-13T13:56:46Z

Codecov Report

Base: 84.74% // Head: 84.14% // Decreases project coverage by -0.60% ⚠️

Coverage data is based on head (4087c3e) compared to base (14c3e05).
Patch coverage: 87.76% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   84.74%   84.14%   -0.60%     
==========================================
  Files          27       27              
  Lines        3415     3476      +61     
  Branches      713      714       +1     
==========================================
+ Hits         2894     2925      +31     
- Misses        408      435      +27     
- Partials      113      116       +3

Impacted Files	Coverage Δ
python/lsst/resources/tests.py	`97.57% <66.66%> (-0.39%)`	⬇️
python/lsst/resources/http.py	`77.95% <75.00%> (-2.43%)`	⬇️
tests/test_http.py	`92.51% <89.91%> (-5.23%)`	⬇️
tests/test_s3.py	`87.34% <100.00%> (ø)`
python/lsst/resources/_resourcePath.py	`94.58% <0.00%> (+1.23%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

We are using an external webDAV server to test against. In addition we have the possibility now to test against other real webDAV servers in the develpment setting by specifying via the environment variable LSST_RESOURCES_HTTP_TEST_SERVER_URL the URL of a server to test against. However, since we are no longer mocking responses, in the current situation we cannot fully test cases where the server does not support some features (e.g. a plain HTTP server, or a webDAV server that does not support download a specified range of bytes).

airnandez and others added 11 commits February 7, 2023 10:39

Improve handling of DELETE requests

a9b6f21

Test deletion of a directory raises

0469e84

Add method to explicitly close sessions to avoid warnings when unit t…

a5a5f12

…esting

Improve comments for session retries

e0e5ac1

Comply with conventions for documenting methods

9ac5a1d

Add test case using a local webDAV server

632155b

Minor improvements and corrections of typos

5bbe8c0

Add missing return type annotation

108c893

Raise NotADirectoryError rather than ValueError when not a directory

f7fe03f

This makes it consistent with the other interfaces.

Fix the confusing test case class name for S3 tests

3ce43db

Enable standard testing of HTTP WebDav interface

b79d643

timj reviewed Feb 9, 2023

View reviewed changes

airnandez added 3 commits February 10, 2023 16:54

The root URI to test can include a non-empty path

d52dd73

Ensure the parent directory exists before actually uploading data

554afff

timj approved these changes Feb 10, 2023

View reviewed changes

airnandez added 3 commits February 13, 2023 14:20

Give priority to test against real servers in development setting if …

459bccf

…environment variable is defined

Ensure all webDAV requests download the response body immediately

260b147

Parse status element of PROPFIND response for making easier inspectio…

a7a3d9d

…n of status code

airnandez added 5 commits February 13, 2023 16:11

Simplify path building

3d556fc

Ensure absolute path starts by single slash

67e387f

Internal method _close_sessions() no longer needed

db184ba

Keep naming of test cases consistent

4087c3e

airnandez merged commit 3fd76a0 into main Feb 15, 2023

airnandez deleted the tickets/DM-37917 branch February 15, 2023 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-37917: Add testing against real webDAV server for HttpResourcePath #40

DM-37917: Add testing against real webDAV server for HttpResourcePath #40

airnandez commented Feb 8, 2023

timj left a comment

airnandez commented Feb 10, 2023

timj left a comment

timj Feb 10, 2023

airnandez Feb 14, 2023

timj Feb 10, 2023

airnandez Feb 13, 2023

timj Feb 10, 2023

airnandez Feb 13, 2023 •

edited

timj Feb 10, 2023

airnandez Feb 13, 2023

timj Feb 10, 2023

timj Feb 10, 2023

airnandez Feb 13, 2023

timj Feb 10, 2023

timj Feb 10, 2023

airnandez Feb 13, 2023

codecov bot commented Feb 13, 2023 •

edited

DM-37917: Add testing against real webDAV server for HttpResourcePath #40

DM-37917: Add testing against real webDAV server for HttpResourcePath #40

Conversation

airnandez commented Feb 8, 2023

Checklist

timj left a comment

Choose a reason for hiding this comment

airnandez commented Feb 10, 2023

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airnandez Feb 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2023 • edited

Codecov Report

airnandez Feb 13, 2023 •

edited

codecov bot commented Feb 13, 2023 •

edited