aai-institute / lakefs-spec

An fsspec implementation for the lakeFS project.
http://lakefs-spec.org/
Apache License 2.0
37 stars 4 forks source link

Unexpected test failures locally on `main` #215

Closed nicholasjng closed 6 months ago

nicholasjng commented 6 months ago

Describe the bug

I'm getting some test failures locally that are not there in CI. Since I'm not sure whether that is only because of the lakeFS server version bump, I'm posting this to get opinions.

Almost failures seem to be related to a 403 forbidden when attempting to write on a main branch in tests using reset_branch.

Steps to reproduce

Pull and run lakeFS v1.3.0, install lakefs-spec from main with dev requirements, run tests.

Expected behaviour

No errors.

Logs, screens, other evidence of a bug

============================================================== short test summary info ==============================================================
FAILED tests/test_client_helpers.py::test_reset_branch - PermissionError: 403 cannot write to protected branch: 'lakefs-spec-tests/main/test-39f8e5cc-c6d0-4df4-8db3-e0fa2b8d2c03.txt'
FAILED tests/test_ls.py::test_ls_dircache_remove_uncached - lakefs_sdk.exceptions.ForbiddenException: (403)
FAILED tests/test_ls.py::test_ls_dircache_remove_cached - lakefs_sdk.exceptions.ForbiddenException: (403)
FAILED tests/smoke_tests/test_abstractfilesystem.py::test_touch - OSError: [Errno 5] 500 mime: no media type: 'lakefs-spec-tests/test-84931017/hello.txt'
FAILED tests/smoke_tests/test_abstractfilesystem.py::test_cat_ranges - TypeError: decoding to str: need a bytes-like object, TypeError found
=========================================================== 5 failed, 84 passed in 6.90s ============================================================

Your operating system

MacOS 14

Python version

Python 3.11.6

lakeFS-spec version

main@ad8437a2f

lakeFS server version

v1.3.0

lakeFS SDK version

v1.0.0

How is lakeFS deployed?

local quickstart Docker container

Additional context

No response

Related GitHub issue(s)

No response

AdrianoKF commented 6 months ago

I don't quite understand, why the issue doesn't happen deterministically, but you are right in your suspicion: three test cases try to modify the main branch of the test repo, which now seems to be a protected branch (but I don't know since which lakeFS version). The following patch addresses the issue (you could include it on #214):

diff --git a/tests/test_client_helpers.py b/tests/test_client_helpers.py
index 8b20271..95cae16 100644
--- a/tests/test_client_helpers.py
+++ b/tests/test_client_helpers.py
@@ -261,15 +261,14 @@ def test_reset_branch(
     fs: LakeFSFileSystem,
     repository: str,
     random_file_factory: RandomFileFactory,
+    temp_branch: str,
 ) -> None:
-    branch = "main"
-
     lpath = random_file_factory.make()
     lpath.write_text("Hello")

-    rpath = f"lakefs://{repository}/{branch}/{lpath.name}"
+    rpath = f"lakefs://{repository}/{temp_branch}/{lpath.name}"

     fs.put(str(lpath), rpath)
-    client_helpers.reset_branch(fs.client, repository, branch)
+    client_helpers.reset_branch(fs.client, repository, temp_branch)
     with pytest.raises(FileNotFoundError):
         fs.info(rpath)
diff --git a/tests/test_ls.py b/tests/test_ls.py
index 044baca..92df77b 100644
--- a/tests/test_ls.py
+++ b/tests/test_ls.py
@@ -116,9 +116,12 @@ def test_ls_no_detail(fs: LakeFSFileSystem, repository: str) -> None:
     assert set(fs.dircache.keys()) == {f"{prefix}/data", f"{prefix}/images"}

-def test_ls_dircache_remove_uncached(fs: LakeFSFileSystem, repository: str) -> None:
-    branch = "main"
-    prefix = f"{repository}/{branch}"
+def test_ls_dircache_remove_uncached(
+    fs: LakeFSFileSystem,
+    repository: str,
+    temp_branch: str,
+) -> None:
+    prefix = f"{repository}/{temp_branch}"
     resource = f"{prefix}/"

     try:
@@ -133,12 +136,15 @@ def test_ls_dircache_remove_uncached(fs: LakeFSFileSystem, repository: str) -> N
         listing_post = fs.ls(resource)
         assert len(listing_post) == len(listing_pre) - 1
     finally:
-        client_helpers.reset_branch(fs.client, repository, branch)
+        client_helpers.reset_branch(fs.client, repository, temp_branch)

-def test_ls_dircache_remove_cached(fs: LakeFSFileSystem, repository: str) -> None:
-    branch = "main"
-    prefix = f"{repository}/{branch}"
+def test_ls_dircache_remove_cached(
+    fs: LakeFSFileSystem,
+    repository: str,
+    temp_branch: str,
+) -> None:
+    prefix = f"{repository}/{temp_branch}"
     resource = f"{prefix}/"

     try:
@@ -149,4 +155,4 @@ def test_ls_dircache_remove_cached(fs: LakeFSFileSystem, repository: str) -> Non
         listing_post = fs.ls(resource)
         assert len(listing_post) == len(listing_pre) - 1
     finally:
-        client_helpers.reset_branch(fs.client, repository, branch)
+        client_helpers.reset_branch(fs.client, repository, temp_branch)
nicholasjng commented 6 months ago

Fixed in #214 (also the cat_ranges fail does not show up anymore).