Improve tracing and assertion messages for reftests #1211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Daniel Carl Jones <[email protected]>
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
dannycjones
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
c-hagem
reviewed
Jan 7, 2025
| assert!(keys.remove(name)); | ||
| assert!( | ||
| keys.remove(name), | ||
| "file name must be in the set of child names in the reference fs", |
There was a problem hiding this comment.
Two nits:
- Maybe include the name if the file (or directory) in the message.
- Should we keep the file naming, even though this can also be a directory? We do the same on L679, so we could leave it.
Suggested change
| "file name must be in the set of child names in the reference fs", | |
| "file {name} name must be in the set of child names in the reference fs", |
There was a problem hiding this comment.
Including the name sounds good! I have some other changes, they're a bit more involved so I won't work on it immediately but I'll include this in those.
There was a problem hiding this comment.
I'll take a look at the file versus directory stuff too, I'd like to make it as accurate as possible - including specifying them as "directory entries" if its not one or the other.
c-hagem
approved these changes
Jan 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds some additional tracing to reftests and makes some adjustments to assertion messages to make it clearer why we assert what we assert and would return a better message when things go wrong.
There are no significant changes, this is primarily readability and debugging improvements.
Does this change impact existing behavior?
No change to behavior of Mountpoint or its libraries.
Does this change need a changelog entry?
No, no behavior change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).