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

ci: tone down fuzzing #474

Merged
merged 2 commits into from
May 15, 2023
Merged

ci: tone down fuzzing #474

merged 2 commits into from
May 15, 2023

Conversation

PaulRBerg
Copy link
Member

Addresses #437.

Note that I did not test the CI workflow just yet.

@PaulRBerg PaulRBerg requested a review from andreivladbrg May 9, 2023 14:16
@PaulRBerg PaulRBerg changed the base branch from main to refactor/extend-status-enum May 9, 2023 14:17
@PaulRBerg
Copy link
Member Author

PaulRBerg commented May 13, 2023

Foundry has recently implemented my feature request (ref foundry-rs/foundry#4085) for selective test fuzz runs. See this PR:

foundry-rs/foundry#4744

It is now possible to override the number of default fuzz runs in a particular test, like this:

/// forge-config: default.fuzz.runs = 100
/// forge-config: ci.fuzz.runs = 500
function test_SimpleFuzzTest(uint256 x) public {
    // --- snip ---
}

We should consider doing this for our tests which fuzz arrays.

Base automatically changed from refactor/extend-status-enum to main May 14, 2023 15:57
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

We should consider doing this for our tests which fuzz arrays

How would you configure it?

test-invariant:
env:
FOUNDRY_INVARIANT_DEPTH: ${{ inputs.invariantDepth || "100" }}
FOUNDRY_INVARIANT_RUNS: ${{ inputs.invariantRuns || "100" }}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the || "100" because we declared the inputs as required: false.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do need the || syntax for the cron-triggered workflow runs. Workflow inputs are not available in cron:

https://stackoverflow.com/a/73495922/3873510

.github/workflows/ci-deep.yml Show resolved Hide resolved
.github/workflows/ci-deep.yml Show resolved Hide resolved
.github/workflows/ci-deep.yml Outdated Show resolved Hide resolved
.github/workflows/ci-deep.yml Outdated Show resolved Hide resolved
@PaulRBerg
Copy link
Member Author

How would you configure it?

I don't know; I couldn't think of any particular test where this would be useful. This is the kind of feature that is best implemented on-the-go, not retrospectively.

So I think we can leave it as is.

@PaulRBerg PaulRBerg merged commit 0912568 into main May 15, 2023
@PaulRBerg PaulRBerg deleted the ci/tone-down branch May 15, 2023 09:10
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