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

feat: stack trace support for setup/deployment and invariant tests #799

Merged
merged 8 commits into from
Feb 11, 2025

Conversation

agostbiro
Copy link
Member

Adds stack trace support by re-executing failed calls for

  • deployment and setup for all types of tests
  • invariant tests

Re-execution on deployment and setup is rather forward, but for invariant tests there are two things to consider:

  1. Previously, we only did tracing on setup and deployment if the invariant test had a setUp method. It greatly simplifies re-execution if we always do tracing on setup and deployment if there are invariant tests. The performance decrease should be negligible, because most invariant test have a setUp method and since they tend to take longer, the overhead is amortized.
  2. The way the revert reason is returned for invariant tests is a bit weird in Foundry. Sometimes it's not returned even if there is a revert and when replaying a recorded invariant failure, the rather obscure "replay failure" is returned. I think the reason for this is that they're doing tracing by default on re-execution and display the revert reason from the traces instead of TestResult::reason. I opted to change this behavior so that the revert reason in the test result matches the stack trace which Hardhat relies on. Note that there are some failing Foundry integration tests related to this which I'll fix if we agree on this point.

TODOS left for stack traces after this PR:

  • Re-execution for fuzz tests
  • Make sure re-execution is safe in fork mode based on whether the block spec is cacheable
  • Detect if an impure cheatcode was used

No changeset as this gets released to @ignored/edr.

Copy link

changeset-bot bot commented Feb 10, 2025

⚠️ No Changeset found

Latest commit: 223c4b0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@agostbiro agostbiro requested a review from Wodann February 10, 2025 19:15
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 10, 2025 19:15 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 10, 2025 19:15 — with GitHub Actions Inactive
@agostbiro agostbiro requested a review from fvictorio February 10, 2025 19:15
@agostbiro agostbiro self-assigned this Feb 10, 2025
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 10, 2025 19:16 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 10, 2025 19:17 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 10, 2025 19:17 — with GitHub Actions Inactive
@Wodann
Copy link
Member

Wodann commented Feb 10, 2025

The way the revert reason is returned for invariant tests is a bit weird in Foundry. Sometimes it's not returned even if there is a revert and when replaying a recorded invariant failure, the rather obscure "replay failure" is returned. I think the reason for this is that they're doing tracing by default on re-execution and display the revert reason from the traces instead of TestResult::reason. I opted to change this behavior so that the revert reason in the test result matches the stack trace which Hardhat relies on. Note that there are some failing Foundry integration tests related to this which I'll fix if we agree on this point.

Based on this context, it's hard for me to agree or disagree. Maybe we can discuss it tomorrow during our sync?

Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

The code changes look good to me. As I mentioned previously, it would be good to discuss the approach taken during the sync, to help me verify my understanding before I can agree/disagree.

I also left one question and one comment on the code changes themselves.

crates/edr_solidity_tests/src/runner.rs Outdated Show resolved Hide resolved
js/helpers/package.json Show resolved Hide resolved
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 13:17 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 13:17 — with GitHub Actions Inactive
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 13:18 — with GitHub Actions Error
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 13:18 — with GitHub Actions Error
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 13:18 — with GitHub Actions Error
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 13:28 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 13:28 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 13:28 — with GitHub Actions Inactive
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 13:28 — with GitHub Actions Error
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 13:29 — with GitHub Actions Error
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 14:07 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 14:07 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 14:07 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 14:08 — with GitHub Actions Inactive
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 14:08 — with GitHub Actions Failure
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 16:51 — with GitHub Actions Error
@agostbiro agostbiro had a problem deploying to github-action-benchmark February 11, 2025 16:51 — with GitHub Actions Error
@agostbiro
Copy link
Member Author

@Wodann thanks for the review! As discussed, pinging for final review of the PR. These are the changes since you've reviewed:

Invariant test-related fixes:

Fuzz tests:

@agostbiro agostbiro requested a review from Wodann February 11, 2025 16:53
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 16:54 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 16:54 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 16:58 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 16:58 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark February 11, 2025 16:58 — with GitHub Actions Inactive
Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Final changes look good to me

@agostbiro agostbiro merged commit 43e9b9c into feat/solidity-tests Feb 11, 2025
41 checks passed
@agostbiro agostbiro deleted the feat/stack-trace-replay branch February 11, 2025 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants