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.

Sign up for GitHub

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

Jump to bottom

Rename fuzz to convention #48

Merged
merged 11 commits into from
Aug 18, 2023
Merged

Rename fuzz to convention #48

merged 11 commits into from
Aug 18, 2023

Conversation

matYang
Copy link
Collaborator

@matYang matYang commented Aug 17, 2023 •
edited
Loading

Motivation

Fuzz tests show up in snapshot result with test execution time. It can often cause snapshot check failure during CI since execution time can vary between dev's local machine and test env.

Solution

Name fuzz tests as testFuzz_* according to Foundry convention
Skip testFuzz_ in snapshot

@matYang matYang requested a review from connorwstein August 17, 2023 05:33
@matYang matYang requested a review from RensR as a code owner August 17, 2023 05:33
@github-actions
Copy link
Contributor

LCOV of commit cf4a9e1 during Solidity Foundry #162

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit 85f6194 during Solidity Foundry #163

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

RensR
RensR requested changes Aug 17, 2023
View reviewed changes
.github/workflows/solidity-foundry.yml Outdated
@@ -160,7 +160,7 @@ jobs:

- name: Run Forge snapshot
run: |
forge snapshot --check gas-snapshots/${{ matrix.product }}.gas-snapshot
forge snapshot --no-match-test testFuzz_ --check gas-snapshots/${{ matrix.product }}.gas-snapshot
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's try to not edit CI to hide failures

Copy link
Collaborator Author

@matYang matYang Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The snapshot results are like:

MerkleMultiProofTest:testMerkleMulti1of4(bytes32,bytes32,bytes32) (runs: 256, μ: 7037, ~: 7036)
MerkleMultiProofTest:testMerkleMulti2of4(bytes32,bytes32,bytes32,bytes32) (runs: 256, μ: 8714, ~: 8719)
MerkleMultiProofTest:testMerkleMulti3of4(bytes32,bytes32,bytes32,bytes32) (runs: 256, μ: 8795, ~: 8792)
MerkleMultiProofTest:testMerkleMulti4of4(bytes32,bytes32,bytes32,bytes32) (runs: 256, μ: 8772, ~: 8768)

With as much info I can find, these values can be flaky. It would make sense to exclude these tests from snapshot.
foundry-rs/foundry#1081
foundry-rs/foundry#4517
foundry-rs/foundry#5262

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After discussion and further investigation, latest foundry snapshot command does use a static seed, which should lead to deterministic inputs and overall consistent fuzz snapshot results.

@RensR
Copy link
Collaborator

RensR commented Aug 17, 2023 •
edited
Loading

Fuzz tests show up in snapshot result with test execution time. It can often cause snapshot check failure during CI since execution time can vary between dev's local machine and test env.

This is not true, it only shows gas

@matYang matYang changed the title Rename fuzz to convention and skip fuzz in snapshot Rename fuzz to convention Aug 17, 2023
@github-actions
Copy link
Contributor

LCOV of commit 2e545cd during Solidity Foundry #174

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@matYang matYang requested a review from RensR August 17, 2023 15:00
@github-actions
Copy link
Contributor

LCOV of commit 78860b6 during Solidity Foundry #175

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit 9aa3c33 during Solidity Foundry #176

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit 7f88e86 during Solidity Foundry #190

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit a862b83 during Solidity Foundry #194

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit 69ea8d1 during Solidity Foundry #204

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@github-actions
Copy link
Contributor

LCOV of commit 13ebc76 during Solidity Foundry #210

Summary coverage rate:
  lines......: 98.9% (892 of 902 lines)
  functions..: 96.5% (192 of 199 functions)
  branches...: 90.9% (349 of 384 branches)

Files changed coverage rate: n/a

@RensR RensR merged commit 7fdb9bd into ccip-develop Aug 18, 2023
@RensR RensR deleted the chore/exclude-fuzz-tests-from-snapshot branch August 18, 2023 09:11
chainchad pushed a commit to chainchad/ccip that referenced this pull request Jan 24, 2024
@jarnaud
CLI tool for firedrill (smartcontractkit#48)
2cde245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RensR RensR RensR approved these changes

@connorwstein connorwstein Awaiting requested review from connorwstein

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matYang @RensR