Skip to content
This repository was archived by the owner on Apr 11, 2024. It is now read-only.

Allow Asynchronous Calls in setup()/teardown() Functions#174

Open
meanjoe45 wants to merge 7 commits into
bestiejs:mainfrom
meanjoe45:master
Open

Allow Asynchronous Calls in setup()/teardown() Functions#174
meanjoe45 wants to merge 7 commits into
bestiejs:mainfrom
meanjoe45:master

Conversation

@meanjoe45

Copy link
Copy Markdown

This pull request is in reference to issue #70. To start off, this is a breaking change. When the defer option is set to true for a benchmark, both the setup() and teardown() functions require their resolve functions be called (suResolve() and tdResolve() respectively). If the functions are not included with the benchmark, the resolve calls are generated allowing setup() and teardown() functions to be omitted.

I tried to maintain the coding style as you describe, but have no problem changing variable names and such if you feel something else works better.

Currently, the unit tests are updated and are all passing when run with Node.js v4.x. I'm not very versed in running with the browser, so I'd appreciate any assistance people are willing to provide to test out these changes more. This includes verifying that the accuracy of the benchmarks has not been compromised.

Please reference the unit tests for examples of use. Documentation will need to be updated, but wanted to get some validation these changes will be accepted before investing the time to document.

Thanks for all the work you've put into benchmark.js.

@meanjoe45

Copy link
Copy Markdown
Author

I have run the unit tests with Node.js v4.4.2 and v6.9.1 locally on my machine and everything passes. I'm not sure yet why the CI build failed. I'll try to investigate soon, but help is appreciated.

@jdalton jdalton force-pushed the master branch 4 times, most recently from 2f014b6 to c732675 Compare November 22, 2016 05:35
@jdalton jdalton force-pushed the master branch 2 times, most recently from f263303 to a37c36c Compare December 24, 2016 15:51
@shamasis

Copy link
Copy Markdown

@jdalton can we get this merged? this really a required feature where there is no option except async setup / teardown

@daprahamian

Copy link
Copy Markdown

Is there a reason this has not been merged yet?

@gwynjudd

Copy link
Copy Markdown

@jdalton is there anything that can be done to merge this? This is a very useful patch for doing async setup/teardown. Do you want to consider changing it so the breaking nature of the change doesn't occur?

This fixes an issue I found when using benchmark and my library was minified. I don't really understand all of the implications, but what seems to have happened is that the line "deferred.suResolve()" errors as "deferred" was undefined in minified.

By inspection, I noted that similar code was using the "interpolate" function and replaced "#d" with the name of the deferred variable.
@shamasis

Copy link
Copy Markdown

@jdalton ping :-|

Fix benchmark tests when running with minified library code
@vflopes

vflopes commented Aug 9, 2018

Copy link
Copy Markdown

+1
This feature is really awesome and I really need (I think a lot of people too).
Waiting for release.

@rajsite

rajsite commented Oct 9, 2018

Copy link
Copy Markdown

We are trying to use benchmark.js for benchmarking the JS interface to some logic implemented in wasm and are also stuck because our wasm instantiation needs to happen asynchronously. Having async setup would definitely be helpful!

@jszwedko

jszwedko commented Nov 20, 2019

Copy link
Copy Markdown

Just bumping this as I ran into it today given the last movement was ~ a year ago. Is there anything that needs to change for this PR? I'm happy to try to pitch in.

@Feirell

Feirell commented Aug 25, 2020

Copy link
Copy Markdown

@jdalton just another ping, I would help out if there is still something to do to make this happen.

@timoxley

Copy link
Copy Markdown

Bump. This makes accurate async testing difficult

@Uzlopak

Uzlopak commented Mar 9, 2022

Copy link
Copy Markdown

I will adapt this change to my fork

@Finesse

Finesse commented Feb 7, 2024

Copy link
Copy Markdown

Waiting for the PR to be merged...

Awaiter

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.