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.

Sign up for GitHub

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

Jump to bottom

feat(forge): export gas reports #887

Closed
wants to merge 1 commit into from
Closed

feat(forge): export gas reports #887

wants to merge 1 commit into from

Conversation

brockelmore
Copy link
Member

Adds the ability to export gas reports to .gas-report via cli input --gas-report-export or --gre

mattsse
mattsse reviewed Mar 9, 2022
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

--gre is essentially > <output> right?

evm-adapters/src/gas_report.rs
Comment on lines +38 to +39
pub fn new(report_for: Vec<String>, export: bool) -> Self {
Self { report_for, stdout: !export, ..Default::default() }
Copy link
Member

Choose a reason for hiding this comment

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

should this rather be colors?

cli/src/cmd/test.rs
if gas_reporting_export {
let table_view = format!("{}", gas_report);
if let Err(e) = std::fs::write(".gas-report", table_view) {
println!(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
println!(
eprintln!(

@onbjerg onbjerg added the T-feature Type: feature label Mar 10, 2022
@onbjerg onbjerg mentioned this pull request Mar 12, 2022
Closed
8 tasks
@transmissions11 transmissions11 mentioned this pull request Mar 28, 2022
Closed
@gakonst
Copy link
Member

gakonst commented Apr 3, 2022

@transmissions11 what is the recommended way for moving forward with this?

@transmissions11
Copy link
Contributor

transmissions11 commented Apr 3, 2022 •
edited
Loading

imo we should remove forge test --gas-report entirely (and not add this new --gas-report-export flag) and swap out the logic in forge snapshot to output these gas report tables into the .gas-snapshot file instead of the per test breakdown

the telegram seems to agree as well

image

i can probably do that with this PR as a base this week unless anyone wants to frontrun me

@gakonst
Copy link
Member

gakonst commented Apr 3, 2022

Sounds great! Let's go with that then.

@mattsse
Copy link
Member

mattsse commented Apr 3, 2022

supportive of that snapshot is basically an extension of test already so we just need to settle on the --gas-report format then?
we then also need some --color and/or --theme args I believe?

@gakonst
Copy link
Member

gakonst commented Apr 3, 2022

Yeah printing to a file should be without colors!

@gakonst
Copy link
Member

gakonst commented Apr 5, 2022

Closing in support of @transmissions11 integrating this functionality as part of forge snapshot

@gakonst gakonst closed this Apr 5, 2022
@gakonst gakonst deleted the brock/gas_report_export branch April 5, 2022 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse left review comments

Assignees
No one assigned
Labels
T-feature Type: feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brockelmore @gakonst @transmissions11 @mattsse @onbjerg