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(--gas-report): add option to include tests #9232

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Oct 30, 2024

Motivation

Closes #6129
#6129 (comment)
add gas_reports_include_tests = true config to include tests in gas report

Solution

@grandizzy grandizzy changed the title feat(--gas-report): add option to show gas for tests feat(--gas-report): add option to include tests Oct 30, 2024
@grandizzy grandizzy marked this pull request as ready for review October 30, 2024 13:48
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.

Reasonable change, for simplicity I think include_tests is OK but we likely want to document that the gas report will include all of the following:

/// Test function kind.
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
pub enum TestFunctionKind {
    /// `setUp`.
    Setup,
    /// `test*`. `should_fail` is `true` for `testFail*`.
    UnitTest { should_fail: bool },
    /// `test*`, with arguments. `should_fail` is `true` for `testFail*`.
    FuzzTest { should_fail: bool },
    /// `invariant*` or `statefulFuzz*`.
    InvariantTest,
    /// `afterInvariant`.
    AfterInvariant,
    /// `fixture*`.
    Fixture,
}

@grandizzy
Copy link
Collaborator Author

Reasonable change, for simplicity I think include_tests is OK but we likely want to document that the gas report will include all of the following:

was thinking include_tests more of a include test contracts rather than functions (as at the end they're all grouped per contract in report). Can detail this in book when updated with new option, would this work?

@grandizzy grandizzy merged commit c90ea4d into foundry-rs:master Oct 30, 2024
21 checks passed
@grandizzy grandizzy deleted the fix-6129 branch October 30, 2024 14:15
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.

bug(forge test): --gas-report does reporting certain contracts (assembled bytecode in constructor)
2 participants