-
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
fuzz console log & test cases #8387
Conversation
hi @grandizzy , please review this, there's a failed test case in test integration, and i believe it has nothing to do with my pr. btw, I think the docs should be updated if the pr is merged. but I didn't find where to update the docs. please tell me if this work needs to be done, and i'll update it. |
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.
Thank you, nice progress!
I was thinking more along the lines of #3844 requirement, that is to print logs during the test rather than after the test is completed (good for long campaigns where one would like to get feedback during execution). So instead storing logs for all fuzz cases (which would also require more resources for executing these test) we can print logs right after the executor call (if show-execution-logs
enabled), smth like adding
if self.config.show_execution_logs {
for log in decode_console_logs(&call.logs) {
println!(" {log}");
}
}
right after
foundry/crates/evm/evm/src/executors/fuzz/mod.rs
Lines 213 to 222 in 15652dc
pub fn single_fuzz( | |
&self, | |
address: Address, | |
should_fail: bool, | |
calldata: alloy_primitives::Bytes, | |
) -> Result<FuzzOutcome, TestCaseError> { | |
let mut call = self | |
.executor | |
.call_raw(self.sender, address, calldata.clone(), U256::ZERO) | |
.map_err(|_| TestCaseError::fail(FuzzError::FailedContractCall))?; |
Would be nice to extend this to invariant testing as well (would probably require the config to be a general one like show_fuzz_logs
and apply to both fuzz and invariant) but can be handled in a separate PR.
Yep, would be great to follow up with docs update PR after this one is merged. The repo with book is https://github.com/foundry-rs/book |
ok, I got your points. Although |
hi @grandizzy
related code: foundry/crates/forge/bin/cmd/test/mod.rs Lines 455 to 465 in 8b694bb
wdyt |
Yeah, I think the show fuzz logs config could supersede the verbosity, so don't think this is a biggie
Good point, if we're not displaying under their own test then running multiple fuzz tests at the same time could become a mess. So let's stick with the impl of gather logs and displaying them at the end, let's keep for fuzz only for now and extend to invariants after. There's foundry/crates/evm/evm/src/executors/fuzz/mod.rs Lines 25 to 40 in 15652dc
|
do you mean that I should define a new field like |
Yeah, I mean new field in struct instead |
@grandizzy Can we just use And do I need to add something like foundry/crates/forge/bin/cmd/test/mod.rs Lines 105 to 114 in 0116be1
|
yes, that should work
nope, that's not needed |
test fuzz console.log
rename to show_fuzz_logs
add logs field in FuzzTestData add logs field in FuzzTestData
well, I think this is my final commit. please review this to see if any change should be made. @grandizzy |
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, think we can simplify a little bit, pls check
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, thank you! clippy failure not related
Thank you, @grandizzy, for your patience and guidance throughout this PR. You're so meticulous. I'm new to Rust, and every time I thought there was nothing to optimize, your review showed me a better way. 😄👍🏻 Hopefully, this PR will not be reverted again. |
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.
ty
Motivation
As discussed in #3844 , when using fuzz test, console.log doesn't work as expected(display console log info). This PR added a new fuzz config
show_execution_logs
, with which user could decide if the full console log should be displayed. by defaut, the value isfalse
, so it won't affect any fuzz test prior to this change will be made.Solution
A
logs
field is added toCaseOutcome
to keep track of logs that generated in a single fuzz test. And collect logs inFuzzTestData
after each fuzz test finished. then use the configshow_logs
inFuzzConfig
to decide if full console log output should be displayed. if not, theconsole.log
behavior is the same as what it is now.Also, you can use
/// forge-config: default.fuzz.show-logs = true
to enable fuzz test console log per test.4 test cases is added, to test if the new config works as expected.