-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: snapbox migration #8728
Merged
Merged
feat: snapbox migration #8728
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
…s possible, including execute and assertions
DaniPopes
reviewed
Aug 26, 2024
zerosnacks
commented
Aug 26, 2024
mattsse
approved these changes
Aug 27, 2024
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.
lgtm, pending @DaniPopes
DaniPopes
approved these changes
Aug 27, 2024
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.
great
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.
Motivation
The goal of the PR is to sufficiently address the request outlined in:
Closes: #8389
Migrates most tests to use Snapbox.
Cleans up a lot of old helpers. Ideally we can get rid of
stdout_lossy()
all together at some point.Disallows
not_empty
pattern, favoring Snapbox. Helper internals have been refactored to use Snapbox.Solution
Improves a number of tests by applying more thorough match assertions
In order to port the last test cases / make testing more ergonomic:
Known issues:
stdout_lossy
is still used in contexts of capturing the value from two commands and determine equality or casting to a value is required to make a comparison:foundry/crates/cast/tests/cli/main.rs
Lines 1021 to 1025 in bdf48aa
stdout_eq
check.contains
on the output here:foundry/crates/cast/tests/cli/main.rs
Lines 331 to 393 in bdf48aa
Suggestions on what we should add / request upstream:
file!
should support dynamically constructed Path's or default toCARGO_MANIFEST_DIR
, e.g.PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../testdata/etherscan")
stdout_eq
, it has to be statically defined. It is therefore not possible to embed constants inside of the argument passed tostdout_eq
. It is also not possible to have an array of static data passed tostdout_eq
in a loop - it must be unrolled manually.