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

Fuzz testing #1255

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Fuzz testing #1255

wants to merge 16 commits into from

Conversation

immrsd
Copy link
Collaborator

@immrsd immrsd commented Dec 10, 2024

PR Checklist

  • Tests

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Good start on the fuzz testing! I left a few comments

packages/testing/src/math.cairo Outdated Show resolved Hide resolved
packages/token/src/tests/erc20/test_fuzz_erc20.cairo Outdated Show resolved Hide resolved
@@ -48,7 +48,7 @@ jobs:
run: scarb fmt --check --workspace

- name: Run tests and generate coverage report
run: snforge test --workspace --coverage
run: snforge test --workspace --coverage --features fuzzing --fuzzer-runs 10000
Copy link
Member

Choose a reason for hiding this comment

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

I have the feeling that this will soon make tests run timeout in the action.

Copy link
Member

Choose a reason for hiding this comment

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

Tests are not running on the PR btw, when that's fixed I'm curious to see how much time will this take with this few examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reduced the number of fuzzer runs to 500 which seems a reasonable value to me, at least for now

With 10k runs the tests took 1h 55m to complete which is way too much, here are the run results: https://github.com/OpenZeppelin/cairo-contracts/actions/runs/12374506824/job/34537193923

With 1k runs it took 17m 27s to run all the tests. 10 minutes more than our usual test runs.
Results: https://github.com/OpenZeppelin/cairo-contracts/actions/runs/12379685661/job/34554383936

Although it's an acceptable duration, it can increase much when we add more fuzz test cases. So now with 500 runs the tests job takes 12 minutes (5 minutes more than it was without fuzz tests)

packages/testing/src/math.cairo Outdated Show resolved Hide resolved
packages/token/Scarb.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Improvements look good! Just left a final question

Comment on lines +127 to +130
fn test__spend_allowance(supply: u256, spend_amount: u256) {
if supply == Bounded::MAX {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be as comprehensive as we can, what do you think about moving the if statement past the _spend_allowance call and asserting the values don't change if supply == u256 max?

@@ -50,7 +50,7 @@ jobs:
run: scarb fmt --check --workspace

- name: Run tests
run: snforge test --workspace
run: snforge test --workspace --features fuzzing --fuzzer-runs 500
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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.

3 participants