Skip to content
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

Don't call afterEach within beforeEach #46963

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Nov 30, 2021

Otherwise, a new afterEach handler is added for each test case and the number of handlers run grows quadratically (wrt the number of tests in the suite adding the handlers).

Otherwise, a new afterEach handler is added for each test case and the
number of handlers run grows quadratically.
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 30, 2021
@amcasey
Copy link
Member Author

amcasey commented Nov 30, 2021

Before this change, 247177 hooks were executed; after, 152329.

@weswigham
Copy link
Member

Other than numbers of handlers and repeatedly adding handlers being unneeded, does this have some kind of perf impact?

@amcasey
Copy link
Member Author

amcasey commented Nov 30, 2021

@weswigham Unclear. Handlers are slower in the module transformation branch and at least part of the problem is that they run more handlers. I'm guessing it has to do with this bug and test ordering, but I'm still investigating. I don't expect any immediate perf win in main though.

@amcasey amcasey merged commit b8ec791 into microsoft:main Nov 30, 2021
@amcasey amcasey deleted the AfterEach branch November 30, 2021 23:08
@amcasey
Copy link
Member Author

amcasey commented Nov 30, 2021

In the module transformation branch, there were 2,883,032 runs before and 151,826 after. Seems like a win. 😄

Edit: And test runtime in that branch dropped from ~1200s to ~795s.

@rbuckton
Copy link
Member

rbuckton commented Dec 1, 2021

I can't recall if it was jest or mocha, but one of them actually recommends you use afterEach inside of beforeEach, so I find this surprising.

@amcasey
Copy link
Member Author

amcasey commented Dec 1, 2021

@rbuckton My guess was that it was leftover from our previous jest (?) usage. I reviewed a bunch of mocha samples and couldn't find any doing it this way and the trace of hook executions looks a lot more sensible after the change (typically, two before hooks and two after hooks for each test).

@amcasey
Copy link
Member Author

amcasey commented Dec 1, 2021

Also note that, if the old form was correct, some of our other hooks would need updating in the other direction.

@rbuckton
Copy link
Member

rbuckton commented Dec 1, 2021

We've never previously used jest, just mocha.

mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
Otherwise, a new afterEach handler is added for each test case and the
number of handlers run grows quadratically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants