Skip to content

Conversation

@zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Jan 8, 2025

Motivation

Opened for visibility

Ref: #9477 (comment)

Gas snapshots are not accurate outside of isolation mode and we've seen users only run gas snapshots with isolation mode enabled for this reason.

When running a test without --isolate enabled and gas snapshots cheatcodes are active a warning is raised similar to the deprecated cheatcodes (on per cheatcode used per test suite, collected at the end)

This PR also fixes the issue where previously a single test from a test suite or custom group would clear out all the other keys. This has now been changed so that we merge with the previous results, replacing existing keys if they were effected. Note: this comes at the cost of now staining old values. This was deemed preferable based on feedback we received.

Solution

This PR adds a requires keyword for the Cheatcodes spec which lets the developer indicate the prerequisites that are required to run a cheatcode. In this can it is isolation mode for the gas snapshots.

To do:

  • Decide whether to require snapshotValue to also be ran in isolation mode given it is for arbitrary values. Because of its existence users still run into the "running one test overwrites the all related snapshot results"

Not added, this should run regardless

  • Fix tests
  • Test if the skipping works correctly when in-line configs are used
  • Running a single related test should merge its results, replacing only its own result and leaving the others intact (note that this does cause staining!)

…side effect is that any custom group names or file name changes are not reflected - this is delegated to the end user
…has a certain requirement for running it - in the case of gas snapshotting: isolation mode
@zerosnacks zerosnacks changed the title Zerosnacks/skip gas snapshots if not in isolation fix: skip gas snapshots if not in isolation Jan 8, 2025
@zerosnacks zerosnacks changed the title fix: skip gas snapshots if not in isolation fix: skip gas snapshots if not in isolation mode Jan 8, 2025
@zerosnacks zerosnacks changed the title fix: skip gas snapshots if not in isolation mode fix(cheatcodes): skip gas snapshots if not in isolation mode Jan 8, 2025
@zerosnacks zerosnacks added T-feature Type: feature A-cheatcodes Area: cheatcodes labels Jan 8, 2025
@grandizzy grandizzy added the C-forge Command: forge label Jan 9, 2025
@zerosnacks zerosnacks self-assigned this Jan 13, 2025
@willpote
Copy link

Would it be possible to add something to foundry.toml that lets us default to isolation mode? Or defaults to using it only for tests with snapshots? Without this feature, to get accurate gas we have to always remember to run with forge test --isolate.

@zerosnacks
Copy link
Member Author

Would it be possible to add something to foundry.toml that lets us default to isolation mode? Or defaults to using it only for tests with snapshots? Without this feature, to get accurate gas we have to always remember to run with forge test --isolate.

you are currently able to add isolate = true to your foundry.toml or optionally define it in a profile e.g. FOUNDRY_PROFILE=gas so not all tests are ran with isolation enabled

@grandizzy grandizzy added this to the v1.1.0 milestone Feb 11, 2025
@zerosnacks
Copy link
Member Author

zerosnacks commented Feb 18, 2025

Decided this requires a different approach / refactor as this PR would not lead to a better user experience as it requires inline configuration of isolate being set manually at every function using gas snapshot cheatcodes or run all tests in isolation.

Ideally isolation mode is enabled by default if we detect a gas snapshot cheatcode being used. Considering this is not feasible a better alternative is to enable isolation mode if the user explicitly requests a gas report to be generated.

Since this PR was opened a new gas_snapshot_emit configuration item was added to disable writing the snapshots to disk but this is enabled by default. If gas_snapshot_emit was simply kept as true all tests would be running in isolation mode by default causing a performance regression.

I propose the following breaking changes instead:

  • rename gas_snapshot_emit to gas_snapshot and set to false by default
  • only run gas_snapshot if passed as flag, similar to --gas-report
  • when gas_snapshot is enabled we automatically configure isolation mode as it is a requirement for accurate gas
  • rename FORGE_SNAPSHOT_CHECK into FORGE_GAS_SNAPSHOT_CHECK
  • rename FORGE_SNAPSHOT into FORGE_GAS_SNAPSHOT
  • there is no inline configuration to change this behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cheatcodes Area: cheatcodes C-forge Command: forge T-feature Type: feature

Projects

Status: Completed

Development

Successfully merging this pull request may close these issues.

4 participants