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.

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

fix(cheatcodes): improve fork cheatcodes messages #9141

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

grandizzy
Copy link
Collaborator

Motivation

Closes #8908

follow up on #8936 (review) not sure what's the best solution here but I hit this while doing some tests, the issue is with wrapped errors that are not displaying entirely when failed from cheatcodes and propagated to test result, #8936 could be a better solution as will handle all other cases if any...

@yash-atreya @mattsse @klkvr

Solution

@grandizzy grandizzy marked this pull request as ready for review October 18, 2024 14:12
.ecx
.db
.create_select_fork(fork, &mut ccx.ecx.env, &mut ccx.ecx.journaled_state)
.map_err(|err| fmt_err!("{err:?}"))?;
Copy link
Member

Choose a reason for hiding this comment

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

If these are for eyre errors then i would revert these changes and update the implementation of From<eyre::Error>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, this would be also fixing same issue for non vm errors like here #8908 (comment) if I get this right?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Collaborator Author

@grandizzy grandizzy Oct 21, 2024

Choose a reason for hiding this comment

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

@DaniPopes pls check c0ea850 displaying as
image

@DaniPopes
Copy link
Member

DaniPopes commented Oct 19, 2024

I'm not a fan of multi-line errors because it skews formatting for the whole test report, it would be better if it was flattened into one line

@grandizzy
Copy link
Collaborator Author

I'm not a fan of multi-line errors because it skews formatting for the whole test report, it would be better if it was flattened into one line

Agree, multi line looks good for cli reports but not for tests.

@DaniPopes
Copy link
Member

You can get an iterator over the error messages with .chain()

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Much better 👍

@grandizzy grandizzy merged commit 2044fae into foundry-rs:master Oct 21, 2024
21 checks passed
@grandizzy grandizzy deleted the issue-8908 branch October 21, 2024 14:22
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
@grandizzy grandizzy added T-feature Type: feature C-forge Command: forge labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug: logs emitted on RPC failures when forking are not descriptive enough
5 participants