DM-33394: Refactor the tests

doc/changes/DM-33394.misc.rst

@@ -0,0 +1 @@
+Reorganize test code to enhance code reuse and allow new schemes to make use of existing tests.

python/lsst/resources/_resourcePath.py

@@ -800,15 +800,18 @@ def as_local(self) -> Iterator[ResourcePath]:
                ospath = local.ospath
         """
         if self.dirLike:
-            raise TypeError(f"Directory-like URI {self} cannot be fetched as local.")
+            raise IsADirectoryError(f"Directory-like URI {self} cannot be fetched as local.")
         local_src, is_temporary = self._as_local()
         local_uri = ResourcePath(local_src, isTemporary=is_temporary)
 
         try:
             yield local_uri
         finally:
-            # The caller might have relocated the temporary file
-            if is_temporary and local_uri.exists():
+            # The caller might have relocated the temporary file.
+            # Do not ever delete if the temporary matches self
+            # (since it may have been that a temporary file was made local
+            # but already was local).
+            if self != local_uri and is_temporary and local_uri.exists():
                 local_uri.remove()
 
     @classmethod
@@ -1216,7 +1219,7 @@ def open(
         when `prefer_file_temporary` is `False`.
         """
         if self.dirLike:
-            raise TypeError(f"Directory-like URI {self} cannot be opened.")
+            raise IsADirectoryError(f"Directory-like URI {self} cannot be opened.")
         if "x" in mode and self.exists():
             raise FileExistsError(f"File at {self} already exists.")
         if prefer_file_temporary:

python/lsst/resources/file.py

@@ -81,9 +81,9 @@ def _as_local(self) -> Tuple[str, bool]:
         path : `str`
             The local path to this file.
         temporary : `bool`
-            Always returns `False` (this is not a temporary file).
+            Always returns the temporary nature of the input file resource.
         """
-        return self.ospath, False
+        return self.ospath, self.isTemporary
 
     def read(self, size: int = -1) -> bytes:
         """Return the entire content of the file as bytes."""
@@ -103,11 +103,20 @@ def write(self, data: bytes, overwrite: bool = True) -> None:
             f.write(data)
 
     def mkdir(self) -> None:
-        """Make the directory associated with this URI."""
-        if not os.path.exists(self.ospath):
+        """Make the directory associated with this URI.
+
+        An attempt will be made to create the directory even if the URI
+        looks like a file.
+
+        Raises
+        ------
+        NotADirectoryError:
+            Raised if a non-directory already exists.
+        """
+        try:
             os.makedirs(self.ospath, exist_ok=True)
-        elif not os.path.isdir(self.ospath):
-            raise FileExistsError(f"URI {self} exists but is not a directory!")
+        except FileExistsError:
+            raise NotADirectoryError(f"{self.ospath} exists but is not a directory.") from None
 
     def isdir(self) -> bool:
         """Return whether this URI is a directory.
@@ -179,23 +188,38 @@ def transfer_from(
 
             if not os.path.exists(local_src):
                 if is_temporary:
-                    msg = f"Local file {local_uri} downloaded from {src} has gone missing"
+                    if src == local_uri:
+                        msg = f"Local temporary file {src} has gone missing."
+                    else:
+                        # This will not happen in normal scenarios.
+                        msg = f"Local file {local_uri} downloaded from {src} has gone missing"
                 else:
                     msg = f"Source URI {src} does not exist"
                 raise FileNotFoundError(msg)
 
             # Follow soft links
             local_src = os.path.realpath(os.path.normpath(local_src))
 
-            # All the modes involving linking use "link" somewhere
-            if "link" in transfer and is_temporary:
+            # Creating a symlink to a local copy of a remote resource
+            # should never work. Creating a hardlink will work but should
+            # not be allowed since it is highly unlikely that this is ever
+            # an intended option and depends on the local target being
+            # on the same file system as was used for the temporary file
+            # download.
+            # If a symlink is being requested for a local temporary file
+            # that is likely undesirable but should not be refused.
+            if is_temporary and src != local_uri and "link" in transfer:
                 raise RuntimeError(
                     f"Can not use local file system transfer mode {transfer} for remote resource ({src})"
                 )
+            elif is_temporary and src == local_uri and "symlink" in transfer:
+                log.debug(
+                    "Using a symlink for a temporary resource may lead to unexpected downstream failures."
+                )
 
-            # For temporary files we can own them
+            # For temporary files we can own them if we created it.
             requested_transfer = transfer
-            if is_temporary and transfer == "copy":
+            if src != local_uri and is_temporary and transfer == "copy":
                 transfer = "move"
 
             # The output location should not exist unless overwrite=True.
@@ -296,7 +320,7 @@ def transfer_from(
             # This was an explicit move requested from a remote resource
             # try to remove that remote resource. We check is_temporary because
             # the local file would have been moved by shutil.move already.
-            if requested_transfer == "move" and is_temporary:
+            if requested_transfer == "move" and is_temporary and src != local_uri:
                 # Transactions do not work here
                 src.remove()
 

python/lsst/resources/http.py

@@ -438,7 +438,16 @@ def transfer_from(
                 transfer,
             )
 
-        if self.exists():
+        # Short circuit if the URIs are identical immediately.
+        if self == src:
+            log.debug(
+                "Target and destination URIs are identical: %s, returning immediately."
+                " No further action required.",
+                self,
+            )
+            return
+
+        if self.exists() and not overwrite:
             raise FileExistsError(f"Destination path {self} already exists.")
 
         if transfer == "auto":

python/lsst/resources/s3.py

@@ -154,7 +154,7 @@ def mkdir(self) -> None:
             raise ValueError(f"Bucket {self.netloc} does not exist for {self}!")
 
         if not self.dirLike:
-            raise ValueError(f"Can not create a 'directory' for file-like URI {self}")
+            raise NotADirectoryError(f"Can not create a 'directory' for file-like URI {self}")
 
         # don't create S3 key when root is at the top-level of an Bucket
         if not self.path == "/":
@@ -173,7 +173,14 @@ def _as_local(self) -> Tuple[str, bool]:
         """
         with tempfile.NamedTemporaryFile(suffix=self.getExtension(), delete=False) as tmpFile:
             with time_this(log, msg="Downloading %s to local file", args=(self,)):
-                self.client.download_fileobj(self.netloc, self.relativeToPathRoot, tmpFile)
+                try:
+                    self.client.download_fileobj(self.netloc, self.relativeToPathRoot, tmpFile)
+                except (
+                    ClientError,
+                    self.client.exceptions.NoSuchKey,
+                    self.client.exceptions.NoSuchBucket,
+                ) as err:
+                    raise FileNotFoundError(f"No such resource: {self}") from err
         return tmpFile.name, True
 
     @backoff.on_exception(backoff.expo, all_retryable_errors, max_time=max_retry_time)
@@ -214,6 +221,15 @@ def transfer_from(
                 transfer,
             )
 
+        # Short circuit if the URIs are identical immediately.
+        if self == src:
+            log.debug(
+                "Target and destination URIs are identical: %s, returning immediately."
+                " No further action required.",
+                self,
+            )
+            return
+
         if not overwrite and self.exists():
             raise FileExistsError(f"Destination path '{self}' already exists.")
 
@@ -232,9 +248,12 @@ def transfer_from(
                 "Key": src.relativeToPathRoot,
             }
             with time_this(log, msg=timer_msg, args=timer_args):
-                self.client.copy_object(
-                    CopySource=copy_source, Bucket=self.netloc, Key=self.relativeToPathRoot
-                )
+                try:
+                    self.client.copy_object(
+                        CopySource=copy_source, Bucket=self.netloc, Key=self.relativeToPathRoot
+                    )
+                except (self.client.exceptions.NoSuchKey, self.client.exceptions.NoSuchBucket) as err:
+                    raise FileNotFoundError("No such resource to transfer: {self}") from err
         else:
             # Use local file and upload it
             with src.as_local() as local_uri:
@@ -316,9 +335,9 @@ def walk(
         # Directories do not exist so we can't test for them. If no files
         # or directories were found though, this means that it effectively
         # does not exist and we should match os.walk() behavior and return
-        # [].
+        # immediately.
         if not dirnames and not files_there:
-            yield []
+            return
         else:
             yield self, dirnames, filenames