Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt storage tests for changes in fsspec (#1819) (#1679) #1970

Closed
wants to merge 2 commits into from

Conversation

AdamWill
Copy link

@AdamWill AdamWill commented Jun 17, 2024

This is adapted from the fixes that were rolled into #1785 for the v3 branch.

TODO:

  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

Deleted TODO items are not applicable as this changes no code, only tests.

…r-developers#1679)

This is adapted from the fixes that were rolled into
zarr-developers#1785 for the
v3 branch.

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill
Copy link
Author

AdamWill commented Jun 17, 2024

On v3 the check in test_hierarchy was commented out entirely, but that seems excessive to me. The comment # no exception raised if path does not exist or is leaf indicates the primary point of these checks is to make sure the listdir calls run without causing an assertion, and we can still do that, and check that the result is one of the two we know are possible.

@dstansby
Copy link
Contributor

Thanks for this! To get the tests passing, I think the version of fsspec in requirements_dev_optional.txt will need increasing to a version that's compatible with the new tests.

@AdamWill
Copy link
Author

ugh, it's been a while, but I think the change is supposed to work with both old and new fsspec? I'll try and find a minute to remember it...

@jhamman
Copy link
Member

jhamman commented Oct 11, 2024

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

@jhamman jhamman closed this Oct 11, 2024
@AdamWill
Copy link
Author

yeah, looking at it again, this is intended to work both ways.

@AdamWill
Copy link
Author

eh, but looking at the test results, it didn't. not sure why not, i'd have to remember all this stuff again. sigh.

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.

3 participants