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: improve forge test configuration for checking gas section snapshots #9734

Closed
2 tasks done
turbocrime opened this issue Jan 21, 2025 · 1 comment · Fixed by #9791
Closed
2 tasks done

feat: improve forge test configuration for checking gas section snapshots #9734

turbocrime opened this issue Jan 21, 2025 · 1 comment · Fixed by #9791
Assignees
Labels
C-forge Command: forge T-feature Type: feature

Comments

@turbocrime
Copy link
Contributor

turbocrime commented Jan 21, 2025

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge Version: 0.3.1-nightly Commit SHA: fea3885 Build Timestamp: 2025-01-21T16:53:17.552998000Z (1737478397) Build Profile: release

What version of Foundryup are you on?

No response

What command(s) is the bug in?

forge

Operating System

macOS (Apple Silicon)

Describe the bug

the env var FORGE_SNAPSHOT_CHECK is available to enable checking section snapshots (those snapshots defined with vm.startSnapshotGas). but its use is not consistent with other configuration.

// Check for differences in gas snapshots if `FORGE_SNAPSHOT_CHECK` is set.
// Exiting early with code 1 if differences are found.
if std::env::var("FORGE_SNAPSHOT_CHECK").is_ok() {

problems

  • there is no equivalent command-line flag
  • configuration is not located with other env var access. a call to std::env::var appears directly in the conditional
  • the value of the env var is not parsed (so FORGE_SNAPSHOT_CHECK="false" or even FORGE_SNAPSHOT_CHECK="" creates the same behavior as FORGE_SNAPSHOT_CHECK="true")

consistency

it seems like figment is intended to provide access to configuration, but it is inconsistently used across the project. there are other examples of configuration that does not arrive via figment.

in this case, the forge test command uses clap arg macros for command-line arguments, and some of the arg identify env variables as sources of argument values. the defined clap args are then merged into figment with merge_impl_figment_convert

however, FORGE_SNAPSHOT_CHECK has no associated arg, because it is accessed directly, and is not merged.

suggestions

  • access to env should be collected in one location
  • a command-line option should be available to override any env configuration
  • env booleans should be parsed for value instead of a simple presence condition

defining clap args for snapshot check could achieve the suggestions.

@turbocrime turbocrime added T-bug Type: bug T-needs-triage Type: this issue needs to be labelled labels Jan 21, 2025
@github-project-automation github-project-automation bot moved this to Todo in Foundry Jan 21, 2025
@zerosnacks zerosnacks added T-feature Type: feature C-forge Command: forge labels Jan 22, 2025
@zerosnacks zerosnacks changed the title improve forge test configuration for checking gas section snapshots feat: improve forge test configuration for checking gas section snapshots Jan 22, 2025
@zerosnacks zerosnacks removed T-bug Type: bug T-needs-triage Type: this issue needs to be labelled labels Jan 22, 2025
@zerosnacks
Copy link
Member

Agreed, in favor of the proposed change to add a bool entry snapshot_check in config w/ FORGE_SNAPSHOT_CHECK

We can then update this check to use the parsed value whilst maintaining backwards compatibility

The Foundry book would need to have a minor update: https://book.getfoundry.sh/forge/gas-section-snapshots

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

2 participants