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

Removed inlining of fn/setup/teardown inside a try/catch #104

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jayphelps
Copy link
Contributor

While getting accustomed to the code I noticed an oddity.

I'm unsure why, if it's checked as a function, fn/setup/teardown is then inlined and wrapped in a try/catch, with the catch then calling it as a function. When running the tests, I found no case where it seemed to be intentional. There may be a rare case it doesn't throw and works as expected (with different scoping rules), but I can't see a benefit to it.

Removing it makes it easier for someone to make the hooks async; possibly myself..as I'm experimenting, but no promises.

I traced the code back to the first introduction of this type of logic, here bb1511c but it's still unclear to me why it's needed since if it's not a function, it would be inlined in the else clause. Certainly, please correct me if I'm wrong. The tests still pass, so I would argue there's some missing tests, if this behavior is needed. 🍻

@jdalton jdalton added the change label Jul 25, 2015
@jdalton jdalton force-pushed the master branch 6 times, most recently from fbdc4d3 to ed61695 Compare January 4, 2016 04:43
@jdalton jdalton force-pushed the master branch 6 times, most recently from 17cca63 to 0729f71 Compare January 28, 2016 08:10
@jdalton jdalton force-pushed the master branch 5 times, most recently from be29f7c to 3ce818c Compare May 23, 2016 17:50
@jdalton jdalton force-pushed the master branch 2 times, most recently from bb3864c to b5cb520 Compare July 18, 2016 17:00
@jdalton jdalton force-pushed the master branch 2 times, most recently from 3aca059 to 189eda5 Compare July 24, 2016 22:34
@jdalton jdalton force-pushed the master branch 5 times, most recently from da00022 to e156ebc Compare August 2, 2016 03:11
@jdalton jdalton force-pushed the master branch 2 times, most recently from b08353e to f118abf Compare August 8, 2016 16:47
@jdalton jdalton force-pushed the master branch 3 times, most recently from 572f88b to 10319e4 Compare September 26, 2016 04:41
@jdalton jdalton force-pushed the master branch 4 times, most recently from 7d00d55 to 115bc9d Compare October 6, 2016 15:30
@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
@jdalton jdalton force-pushed the master branch 2 times, most recently from fa368bc to 7c31726 Compare February 6, 2017 06:03
@jdalton jdalton force-pushed the master branch 2 times, most recently from e87b3c3 to 70a8b8b Compare March 6, 2017 02:46
@prantlf
Copy link

prantlf commented Jan 31, 2020

I think, than the purpose was to insert a common code to the benchmark, so that you do not have to repeat it. If you want to include the code in the measurement, you will use the options setup and teardown. If you want to exclude the code from the measurement, you will listen to the events start and complete. I do not know either, why this code is guarded by a try & catch.

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

Successfully merging this pull request may close these issues.

3 participants