-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add runSync
method to Bench
to force benchmarks to be synchronous
#210
Conversation
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.
Looks good at a first glance.
commit: |
runSync
method to Bench
to force benchmarks to be synchronousrunSync
method to Bench
to force benchmarks to be synchronous
59f21ee
to
91cccfb
Compare
src/bench.ts
Outdated
runSync (): Task[] { | ||
if (this.concurrency != null) { | ||
throw new Error('Cannot use `concurrency` option when using `runSync`') | ||
} |
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.
Please do the same for the runSync
method in the Task
class and we're good to go (yes, it's bad to have Task
exported not only as a type but unfortunately the API have already been widely abused)
91cccfb
to
34b1feb
Compare
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.
Ready to land
Thanks for the reviews, @jerome-benoit !! |
It also validates that all the hooks (beforeAll, beforeEach, etc.) are synchronous as well and makes the benchmark fail if any of them is async (returns a promise-like object).
I did a slight refactor to extract logic common to both sync and async, and duplicated the tests that made sense to test for flavors.
closes #202