-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(forge
) - Test scaffolding
#5495
Conversation
HI @Evalir - Let me know if anything else to be done. Thanks! |
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.
looks good! supportive—pretty harmless addition. wdyt @mattsse ?
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.
just some quick nits, but lgtm!
cli/src/cmd/forge/generate/mod.rs
Outdated
.replace("{instance_name}", &instance_name); | ||
|
||
// Create the test directory if it doesn't exist. | ||
fs::create_dir_all("test").unwrap(); |
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.
Could we avoid unwrap
here? We can just use ?
and maybe map_err
as well to make it a tad more descriptive.
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.
Absolutely! I looked at some other files and figured there is wrapper around fs foundry_common::fs
so I have swapped the implementation.
cli/src/cmd/forge/generate/mod.rs
Outdated
let test_file_path = Path::new("test").join(format!("{}.t.sol", contract_name)); | ||
|
||
// Write the test content to the test file. | ||
fs::write(&test_file_path, test_content).unwrap(); |
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.
ditto on unwrap
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.
fixed too
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.
amazing, thank you!
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 looks pretty good, nicely done
I like the idea of using templates and code gen in my use of foundry. What would also be nice is if a local project specific template could get used if it say exists in |
that'd be a nice feature, please open a separate issue for this |
* feat: foundry-rs#5466 - Test scaffolding * reafactor: removed return * chore: fmt * refactor: named imports * refactor: std::fs -> foundry_common::fs --------- Co-authored-by: Rahul Ravindran <ravindranrahul@users.noreply.github.com> Co-authored-by: Enrique Ortiz <hi@enriqueortiz.dev>
Note
I have exactly 3 days of Rust experience. So I had to wing this PR. Please excuse if I have missed any obvious expectations. BTW rust blown away by the rust compiler 🤯
Motivation
Creating test file typically involves repeated, verbose steps. This PR introduces a new forge sub command to help scaffold generation of test file - one of many features discussed in detail in this issue: #5466
Solution
A new forge sub command has been introduced
forge generate test --contract-name <CONTRACT_NAME>
. For a given contract it does the following:tests
directory.A sub command called generate has intentionally been added to accommodate more potential scaffolds in the future - like generating Interface for a given contract which also is a cumbersome exercise.
cc: @mattsse @Evalir @0x-r4bbit