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

feat: add gas_snapshot_check flag to config, fix FORGE_SNAPSHOT_CHECK behavior #9791

Merged
merged 9 commits into from
Jan 30, 2025

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Jan 30, 2025

Motivation

Closes: #9734

Unblocks: #9697 / #9710

Solution

Maintains backwards compatibility of FORGE_SNAPSHOT_CHECK with the added restriction of it requiring a boolean value (FORGE_SNAPSHOT_CHECK=true) in accordance to the other configuration items.

Evaluates in the following order:

Default is false

  • We check whether FORGE_SNAPSHOT_CHECK=true is set
  • We check whether gas_snapshot_check=true is set in the config
  • Last we check --gas-snapshot-check=<bool> which takes preference over all, both true and false

Marked as minor breaking due to FORGE_SNAPSHOT_CHECK now explicitly checking for truthiness, not just existence

@zerosnacks zerosnacks added the T-likely-breaking Type: requires changes that can be breaking label Jan 30, 2025
@zerosnacks zerosnacks changed the title feat: add gas_snapshot_check flag to config, update FORGE_SNAPSHOT_CHECK to feat: add gas_snapshot_check flag to config, fix FORGE_SNAPSHOT_CHECK behavior Jan 30, 2025
@zerosnacks zerosnacks marked this pull request as ready for review January 30, 2025 13:10
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, have minor nit re snapshot check option

crates/forge/bin/cmd/test/mod.rs Outdated Show resolved Hide resolved
@zerosnacks zerosnacks requested a review from grandizzy January 30, 2025 13:58
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zerosnacks zerosnacks enabled auto-merge (squash) January 30, 2025 14:54
@zerosnacks zerosnacks merged commit fe92e7e into master Jan 30, 2025
22 checks passed
@zerosnacks zerosnacks deleted the zerosnacks/add-snapshot-flag-to-config branch January 30, 2025 15:09
zerosnacks added a commit to foundry-rs/book that referenced this pull request Jan 30, 2025
@zerosnacks zerosnacks added the C-forge Command: forge label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-likely-breaking Type: requires changes that can be breaking
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

feat: improve forge test configuration for checking gas section snapshots
2 participants