-
-
Notifications
You must be signed in to change notification settings - Fork 356
don't test self-deletion in stateful tests #3243
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
don't test self-deletion in stateful tests #3243
Conversation
src/zarr/testing/stateful.py
Outdated
group_path = data.draw( | ||
st.sampled_from(sorted(self.all_groups)), label="Group deletion target" | ||
) | ||
members = tuple(filter(lambda v: "/" in v, sorted(self.all_groups))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this will filter out
foo/bar
too no?
members = tuple(filter(lambda v: "/" in v, sorted(self.all_groups))) | |
members = tuple(filter(lambda v: v == "/", sorted(self.all_groups))) |
- Should this op be an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will filter out foo/bar too no?
It will reject any member name that doesn't contain "/", so it will keep foo/bar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about how this is working. Can you add a comment please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in 724e3c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me know if this explanation needs more context, or if there's just a better way to get the same result -- I'm assuming it's a given that the name of the root group does not contain a "/" character.
closes #3244 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3243 +/- ##
==========================================
+ Coverage 92.25% 94.62% +2.37%
==========================================
Files 78 78
Lines 8689 8690 +1
==========================================
+ Hits 8016 8223 +207
+ Misses 673 467 -206
🚀 New features to boost your workflow:
|
This PR prevents the stateful group tests from deleting the root group
TODO:
docs/user-guide/*.rst
changes/