feat(forge): add contract ignore list for gas reports#2528
feat(forge): add contract ignore list for gas reports#2528gakonst merged 2 commits intofoundry-rs:masterfrom
Conversation
onbjerg
left a comment
There was a problem hiding this comment.
Some minor nits, otherwise good to me so far :)
|
Let me know if something is blocking you :) |
|
As I mentioned here, I need help testing it because I don't know how to write tests for it. So I'd really appreciate if you let me know how can I learn to test new features :) |
|
@derch28 I think the best way currently would be to add a new A good starting point is reading some of the tests in there, figuring out what is closest to what you want to do (e.g. setting |
|
@onbjerg Followed what you said and managed to write some tests :) While testing, I realized that there was a simpler way to solve the issue and that allowed me to delete a function ( So, given that all the previous commits don't represent any logical change (because the PR-ready code was all made in the last one) I'm squashing them and force-pushing the result. Should I edit the "Solution" Section of my first comment and explain how the new solution works there? |
There was a problem hiding this comment.
looks good :) two minor nits, nice one on the clean up
re: updating the original post in the PR, that would be appreciated, mostly because I am going to link to it in the book repo as context for documenting the feature 😄
feel free to take this PR out of draft if you are ready for a final review (mine below)
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
| prj.write_config(Config { | ||
| gas_reports: (vec![ | ||
| "ContractOne".to_string(), | ||
| "ContractTwo".to_string(), | ||
| "ContractThree".to_string(), | ||
| ]), | ||
| gas_reports_ignore: (vec!["ContractThree".to_string()]), | ||
| ..Default::default() | ||
| }); | ||
| cmd.forge_fuse(); | ||
| let third_out = cmd.arg("test").arg("--gas-report").stdout(); | ||
| assert!(third_out.contains("foo") && third_out.contains("bar") && third_out.contains("baz")); |
There was a problem hiding this comment.
ContractThree::baz doesn't look ignored here?
There was a problem hiding this comment.
In this case, "ContractThree" was added to the report list and the ignore list in the same run.
I explained how I decided to handle this behavior in forge/src/gas_report.rs line 53:
"If the user listed the contract in 'gas_reports' (the foundry.toml field) a report for the contract is generated even if it's listed in the ignore list. This is addressed this way because getting a report you don't expect is preferable than not getting one you expect. A warning is printed to stderr indicating the "double listing"."
* feat(forge): add contract ignore list for gas reports * Update cli/tests/it/cmd.rs Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Motivation
See #2464
Solution
Create a new Config field called
gas_reports_ignoreand a new field inGasReportcalledignore. (Default value forgas_reports_ignoreis an empty Vec<String>.)Pass
gas_reports_ignoreas an argument to functionGasReport::newsoGasReports are built with an ignore list.The initialization of the variable
report_contractdecides if the contract has to be reported, using this logic:Were:
In simpler terms: Report the contract every time it's listed on
report_for. Report all contracts ifreport_foris either empty or contains "*". If you're reporting all contracts but one of them is listed onignore, don't report that one.If the user listed the contract in 'gas_reports' (the foundry.toml field) a report for the contract is generated even if it's listed in the ignore list. This is addressed this way because getting a report you don't expect is preferable than not getting one you expect. A warning is printed to stderr indicating the "double listing".