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. 馃嵒

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

None yet

3 participants