-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(forge): add gas snapshotting #204
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
help = "Compare against a snapshot and display changes from the snapshot. Takes an optional snapshot file, [default: .gas-snapshot]", | ||
long | ||
)] | ||
diff: Option<Option<PathBuf>>, |
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.
Is this double option needed?
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.
yeah so this allows us to pass --diff
as flag, in which case we take the .gas-snapshot
file for comparison or
--diff to compare against a specific snapshot file
cli/src/cmd/snapshot.rs
Outdated
|
||
fn run(self) -> eyre::Result<()> { | ||
dbg!(self); | ||
todo!() |
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.
What is the expected behavior for this?
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.
intended for check-snapshot
as mentioned here dapphub/dapptools#761
Adding a dapp check-snapshot command that runs dapp snapshot (but outputs to a temp file instead of .dappshots) and compares it with .dappshots. If they don't match the command exits with an error.
help = "Compare against a snapshot and display changes from the snapshot. Takes an optional snapshot file, [default: .gas-snapshot]", | ||
long | ||
)] | ||
diff: Option<Option<PathBuf>>, |
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.
Is this double option needed?
97e120d
to
efd9a77
Compare
efd9a77
to
b48cd3c
Compare
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 - we can track the prettier table printing separately
Changes
forge.rs
to their dedicated sub modulessnapshot
command, that essentially "inherits" fromtest
snapshot --diff <snapshot>
to print the diff from the current snapshot and an existing snapshot filesnapshot --check <snapshot>
to check the current snapshot and an existing snapshot file, similar tocargo fmt --check
Supports all
forge test
argumentsRef #137