Raise FileNotFoundError from ls() of a missing key path (GH-1330)

pyathena/filesystem/s3.py

@@ -547,6 +547,11 @@ def ls(
         Returns:
             List of S3Object instances (if detail=True) or paths as strings (if detail=False).
 
+        Raises:
+            FileNotFoundError: If the bucket does not exist, or if a key path
+                is neither an object nor a directory, a path that some key
+                starts with followed by ``/``.
+
         Example:
             >>> fs = S3FileSystem()
             >>> fs.ls("s3://my-bucket")  # List objects in bucket
@@ -560,14 +565,20 @@ def ls(
         path = self._strip_protocol(path).rstrip("/")
         if path in ["", "/"]:
             files = self._ls_buckets(refresh)
-        elif versions:
-            files = self._ls_object_versions(path)
         else:
-            files = self._ls_dirs(path, refresh=refresh)
-            if not files and "/" in path:
-                file = self._head_object(path, refresh=refresh)
-                if file:
-                    files = [file]
+            key = S3Path.parse(path).key
+            if versions:
+                files = self._ls_object_versions(path)
+            else:
+                files = self._ls_dirs(path, refresh=refresh)
+                if not files and key:
+                    file = self._head_object(path, refresh=refresh)
+                    if file:
+                        files = [file]
+            # An empty bucket lists nothing, while a missing bucket raises
+            # from the listing.
+            if not files and key:
+                raise FileNotFoundError(path)
         return list(files) if detail else [f.name for f in files]
 
     def _ls_object_versions(self, path: str) -> list[S3Object]:

tests/pyathena/filesystem/test_fsspec_conformance.py

@@ -8,7 +8,6 @@
 """fsspec's conformance tests, run against the S3 filesystems on real S3."""
 
 import contextlib
-import functools
 import posixpath
 import uuid
 
@@ -21,33 +20,6 @@
 from tests.pyathena.conftest import connect
 
 
-def _xfail_missing_ls(test):
-    """Mark an inherited conformance test as failing on GH-1330.
-
-    The test is expected to fail with pytest's ``Failed``, as the
-    ``pytest.raises(FileNotFoundError)`` around ``ls()`` does when nothing is
-    raised, so other errors still fail it. The mark goes on a wrapper, so the
-    test function of fsspec's class stays unmarked.
-
-    Args:
-        test: The test function of fsspec's abstract class.
-
-    Returns:
-        The wrapped test, expected to fail.
-    """
-
-    @functools.wraps(test)
-    def wrapper(*args, **kwargs):
-        return test(*args, **kwargs)
-
-    return pytest.mark.xfail(
-        reason="GH-1330: ls() of a missing key prefix returns [] instead of raising "
-        "FileNotFoundError",
-        raises=pytest.fail.Exception,
-        strict=True,
-    )(wrapper)
-
-
 class S3Fixtures(abstract.AbstractFixtures):
     @pytest.fixture(scope="class")
     def fs(self):
@@ -88,19 +60,15 @@ def fs(self):
 
 
 class TestS3FileSystemCopy(abstract.AbstractCopyTests, S3Fixtures):
-    test_copy_directory_to_new_directory = _xfail_missing_ls(
-        abstract.AbstractCopyTests.test_copy_directory_to_new_directory
-    )
+    pass
 
 
 class TestS3FileSystemGet(abstract.AbstractGetTests, S3Fixtures):
     pass
 
 
 class TestS3FileSystemPut(abstract.AbstractPutTests, S3Fixtures):
-    test_put_directory_to_new_directory = _xfail_missing_ls(
-        abstract.AbstractPutTests.test_put_directory_to_new_directory
-    )
+    pass
 
 
 class TestS3FileSystemPipe(abstract.AbstractPipeTests, S3Fixtures):
@@ -112,16 +80,12 @@ class TestS3FileSystemOpen(abstract.AbstractOpenTests, S3Fixtures):
 
 
 class TestAioS3FileSystemCopy(abstract.AbstractCopyTests, AioS3Fixtures):
-    test_copy_directory_to_new_directory = _xfail_missing_ls(
-        abstract.AbstractCopyTests.test_copy_directory_to_new_directory
-    )
+    pass
 
 
 class TestAioS3FileSystemGet(abstract.AbstractGetTests, AioS3Fixtures):
     pass
 
 
 class TestAioS3FileSystemPut(abstract.AbstractPutTests, AioS3Fixtures):
-    test_put_directory_to_new_directory = _xfail_missing_ls(
-        abstract.AbstractPutTests.test_put_directory_to_new_directory
-    )
+    pass

tests/pyathena/filesystem/test_s3.py

@@ -237,7 +237,13 @@ def test_info_does_not_use_listing_of_path(self):
             pytest.param(
                 False, lambda fs: fs.exists("s3://bucket/d/key", refresh=True), id="exists"
             ),
-            pytest.param(False, lambda fs: fs.ls("s3://bucket/d/key", refresh=True), id="ls"),
+            pytest.param(
+                False,
+                lambda fs: pytest.raises(
+                    FileNotFoundError, fs.ls, "s3://bucket/d/key", refresh=True
+                ),
+                id="ls",
+            ),
             # The listed entry has no version, so it is looked up again.
             pytest.param(True, lambda fs: fs.isfile("s3://bucket/d/key"), id="version_aware"),
         ],
@@ -665,13 +671,24 @@ def test_ls_empty_refresh_evicts_cached_listing(self):
 
         self._serve_keys(fs, ())
 
-        assert fs.ls("s3://bucket/dir", refresh=True) == []
+        with pytest.raises(FileNotFoundError):
+            fs.ls("s3://bucket/dir", refresh=True)
         # The next listing is requested again instead of returning the
         # deleted object from the cache.
         fs._call.reset_mock()
-        assert fs.ls("s3://bucket/dir") == []
+        with pytest.raises(FileNotFoundError):
+            fs.ls("s3://bucket/dir")
         assert fs._call.call_args_list[0].args[0] is fs._client.list_objects_v2
 
+    @pytest.mark.parametrize("path", ["s3://bucket", "s3://bucket/?versionId=v1"])
+    def test_ls_empty_bucket(self, path):
+        # An existing bucket without objects is listed as empty, unlike a key
+        # path that matches nothing.
+        fs = self._make_fs()
+        self._serve_keys(fs, ())
+
+        assert fs.ls(path) == []
+
     def test_find_withdirs_does_not_modify_cached_listing(self):
         fs = self._make_fs()
         fs.dircache[("bucket/dir", "")] = [self._file_object("dir/sub/file")]
@@ -900,9 +917,11 @@ def call(method, **kwargs):
 
         fs._call.side_effect = call
 
-        assert fs.ls("s3://bucket/key", refresh=True) == []
+        with pytest.raises(FileNotFoundError):
+            fs.ls("s3://bucket/key", refresh=True)
         # The next lookups must not return the deleted object and bucket from the cache.
-        assert fs.ls("s3://bucket/key") == []
+        with pytest.raises(FileNotFoundError):
+            fs.ls("s3://bucket/key")
         assert not fs.exists("s3://bucket/key")
         with pytest.raises(FileNotFoundError):
             fs.info("s3://bucket", refresh=True)
@@ -4113,6 +4132,14 @@ def test_ls_versions(self):
         assert actual[2].size == 2
         assert fs.ls("s3://bucket/path", versions=True) == [f.name for f in actual]
 
+    def test_ls_versions_missing_path(self):
+        fs = self._make_fs()
+        fs.version_aware = True
+        fs._call.return_value = {"IsTruncated": False}
+
+        with pytest.raises(FileNotFoundError):
+            fs.ls("s3://bucket/path/missing", versions=True)
+
     def test_ls_versions_object_path_falls_back_to_the_key(self):
         fs = self._make_fs()
         fs.version_aware = True
@@ -4855,7 +4882,9 @@ def test_ls_dirs(self, fs):
 
         assert len(fs.ls(f"{dir_}/prefix")) == 5
         assert len(fs.ls(f"{dir_}/prefix/")) == 5
-        assert len(fs.ls(f"{dir_}/prefix/test_")) == 0
+        # The start of an object's key is neither an object nor a directory.
+        with pytest.raises(FileNotFoundError):
+            fs.ls(f"{dir_}/prefix/test_")
         assert len(fs.ls(f"{dir_}/prefix2")) == 1
 
         test_1 = fs.ls(f"{dir_}/prefix/test_1")