-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore: run benchmarking utility in devnet environment #3166
base: next
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
CodSpeed Performance ReportMerging #3166 will improve performances by ×8.1Comparing Summary
Benchmarks breakdown
|
Coverage Report:
Changed Files:
|
|
||
export const runBenchmark = (name: string, benchmarkFn: () => Promise<void>) => { | ||
bench( | ||
isDevnet ? name : `${name} (x${iterations} times)`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a big enough value add to justify having a function that we need to wrap every test?
We could just import the iterations value from a config file, that is used against the describe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This arose out of a refactor where we had the isDevnet
conditional on each test + adding the iterations per test, but not all tests use the 10 iterations though. So this refactor is the sweet spot I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just floating ideas, what about:
// config.ts
export const isDevnet = process.env.DEVNET_WALLET_PVT_KEY !== undefined;
export const iterations = isDevnet ? 1 : 10;
// bench.test.ts
import { iterations } from './config.ts';
describe('benchmarks', () => {
bench('tests a thing', () => {});
bench('tests another thing', () => {});
}, { iterations });
Then we don't need to augment bench
and can use the other config options if we need them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like having the iterations in the name, as it's explicit to why one bench takes for example: 100ms and another takes 1000ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on our sync today I clarified this refactor with @danielbate
Also to corroborate @petertonysmith94 it's actually required to have these distinctions in the test names so that we get accurate reporting on Codspeed, otherwise the statistics will be convoluted (single iterations mixed with multiple iterations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Summary
This PR introduces devnet benchmarking on master builds, as well as blob transaction deployment testing.
Also previous benchmarking tests are now looped 10 times to have more consistent results.
Checklist