Skip to content

Conversation

@LuQQiu
Copy link
Contributor

@LuQQiu LuQQiu commented Jan 24, 2024

The given dummy in the test is a string type
while the ls() by default is detail=True, which provides a dictionary instead of a string.
This line throws AssertionError when supports_empty_directories=False and the underlying filesystem does not overwrite the default detail to be False

@martindurant
Copy link
Member

Did you find a failing case without this fix?

@LuQQiu LuQQiu force-pushed the fix_abstract_tests branch from 8c7bfd0 to 59f18a3 Compare January 29, 2024 18:35
@LuQQiu LuQQiu force-pushed the fix_abstract_tests branch from 59f18a3 to 1e9c211 Compare January 29, 2024 18:40
@LuQQiu
Copy link
Contributor Author

LuQQiu commented Jan 31, 2024

@martindurant

It's interesting that this code path is not in the existing tests that i could find.
Two conditions are needed:

  1. supports_empty_directories = False
@pytest.fixture
    def supports_empty_directories(self):
        return False
  1. Ls with default detail=True
    def ls(self, path, detail=True, **kwargs):

Tests like local or memory filesystem test do not need to overwrite supports_empty_directories
Tests like S3/Azure filesystem test set the detail=False by default in ls related methods https://github.com/fsspec/s3fs/blob/34a32198188164fd48d4d1abcb267f033d1d1ce1/s3fs/core.py#L966

So my hyphothesis is this line of code never got executed

In my alluxiofs tests which tests against S3 without Alluxio, it will trigger this path because alluxiofs by default ls(detailed=True) following https://github.com/fsspec/filesystem_spec/blob/0c909ee6641381fd8913729b8d803a13a985e5cb/fsspec/spec.py#L313C4-L313C47

@martindurant
Copy link
Member

(please merge from master to get tests to run - there was a dependencies versions issue)

@martindurant martindurant merged commit d110915 into fsspec:master Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants