-
Notifications
You must be signed in to change notification settings - Fork 196
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(cli): replace gas report #1049
Conversation
|
@@ -23,6 +23,12 @@ jobs: | |||
- name: Run tests | |||
run: pnpm test | |||
|
|||
- name: Generate gas reports | |||
run: pnpm gas-report |
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.
This is moved here since the gas report runs best if the tests are already compiled/cached.
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 should add a note about this being a breaking change for existing users of the gas-report
cli to the commit message. Something like:
BREAKING CHANGE:
- make your test files extend `GasReporter` (imported from `@latticexyz/std-contracts/src/test/GasReporter.sol`)
- replace `!gasreport <message>` with `startGasReport(<message>);` and add `endGasReport()` at the end of the code block to be measured
Replaces our slow and error prone approach of generating new test files from magic comments with a Solidity-native approach, inspired by the approach in nomoixyz/vulcan#104.
This hopefully means we can re-open #771 and easier to do #667 (since gas report is now ~just a test output parser)