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: enable snapshot testing #5037

Merged
merged 2 commits into from
Mar 28, 2024
Merged

ci: enable snapshot testing #5037

merged 2 commits into from
Mar 28, 2024

Conversation

agostbiro
Copy link
Collaborator

This PR enables snapshot testing in the CI by checking that the same requests succeed and fail between benchmark runs.

The openzeppelin-contracts and neptune-mutual-blue-protocol scenarios are excluded from snapshot testing, because there is some variance in which calls and transactions succeed or fail. A time-limited investigation found that this variance is related to block timestamps. Since it's difficult to know the expected behavior based on the snapshots, as a follow up we added NomicFoundation/edr#240

This PR also updates the openzeppelin-contracts scenario to a version that was collected after some sporadically failing tests were fixed in the openzeppelin-contracts repo.

Copy link

changeset-bot bot commented Mar 27, 2024

⚠️ No Changeset found

Latest commit: e3e3b57

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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 March 27, 2024 17:14
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview Mar 27, 2024 5:14pm

@agostbiro agostbiro had a problem deploying to github-action-benchmark March 27, 2024 17:14 — with GitHub Actions Failure
@agostbiro agostbiro self-assigned this Mar 27, 2024
@github-actions github-actions bot added the status:ready This issue is ready to be worked on label Mar 27, 2024
@agostbiro agostbiro added no changeset needed This PR doesn't require a changeset and removed status:ready This issue is ready to be worked on labels Mar 27, 2024
@agostbiro agostbiro linked an issue Mar 27, 2024 that may be closed by this pull request
@@ -89,6 +89,14 @@ async function verify(benchmarkResultPath) {
));

for (let scenarioName in snapshotResult) {
// Snapshot testing is unreliable for these scenarios
if (
scenarioName.includes("openzeppelin") ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be related to #5036 (comment)?

Copy link
Collaborator Author

@agostbiro agostbiro Mar 28, 2024

Choose a reason for hiding this comment

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

Thanks good idea, but I don't think so. I updated the openzeppelin scenario to a version in this PR that was collected after Franco's PR to fix the issue in openzeppelin landed: OpenZeppelin/openzeppelin-contracts#4942

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an FYI, when I enabled logging for a repo - that suffers from the described problem - and compared the generated logs, the order of calls wasn't deterministic. This was a sign to me that tests were being executed in parallel.

This might be a relatively straightforward way of testing the hypothesis for other repos.

Copy link
Collaborator

@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.

One discussion but otherwise LGTM

@agostbiro agostbiro temporarily deployed to github-action-benchmark March 28, 2024 09:09 — with GitHub Actions Inactive
@agostbiro agostbiro merged commit e0c08a7 into main Mar 28, 2024
44 of 48 checks passed
@agostbiro agostbiro deleted the edr/ci/enable-snapshot-testing branch March 28, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changeset needed This PR doesn't require a changeset
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Fix regression checks in benchmark CI
2 participants