Hackathon: msal-common - Replaced sinon with jest #7322
Conversation
Robbie-Microsoft
requested review from
sameerag,
tnorling,
hectormmg,
jo-arroyo,
peterzenz,
konstantin-msft,
lalimasharda and
bgavrilMS
as code owners
| }); | ||
|
|
||
| afterAll(() => { | ||
| getEndpointMetadataFromNetworkSpy.mockRestore(); |
There was a problem hiding this comment.
Let's just go ahead and restoreAllMocks here, makes it future proof
| }); | ||
|
|
||
| afterAll(() => { | ||
| timeUtilsSpy.mockRestore(); |
| .mockReturnValue(currTime); | ||
| }); | ||
|
|
||
| afterAll(() => { |
There was a problem hiding this comment.
Suggest setting mocks on a per-test basis (beforeEach/afterEach) rather than a per-block basis (beforeAll/afterAll). It can be really easy to get yourself into trouble with mocks leaking across tests if they're not cleaned up after each test leading to unexpected results.
For simplicity I would suggest setting an afterEach in the topmost describe of each test file which restores all mocks.
There was a problem hiding this comment.
I agree with you about the simplicity, however I've been thinking in terms of efficiency. Creating a "base, constant" mock once before all tests instead of for each test. Does this affect performance? Probably not? I can change it if you want and if everyone prefers simplicity.
I'd have to change some things around, since restoreAllMocks in afterEach wipes everything in the beforeAll.
There was a problem hiding this comment.
My top priority is ensuring mocks don't unexpectedly remain set from one test to another (this would happen if you set one-off mocks inside a specific test but restore in an afterAll). Simplicity is a nice-to-have but secondary. Performance is a non-concern, the test suite runs in just a few seconds as it is.
Change beforeAll -> beforeEach and afterAll -> afterEach and it should behave the same as what you've got but safer.
I went through every msal-common unit test and replaced sinon with jest.
In addition to replacing sinon with jest, I tried to completely overhaul the test files that need work (not re-using code - this is majorly inflating some of the unit test files, etc). I only got through a few files before realizing that I won't finish in time for hackathon week. The files I finished overhauling are:
Logger,ThrottlingUtils,NetworkManagerandSilentFlowClient. Since I'm running out of time and want to replace sinon with jest in msal-browser as well, I gave up overhauling and simply replaced sinon with jest inRefreshTokenClientandAuthorizationCodeClient. Otherwise, the files didn't need overhauling and sinon was simply swapped out for jest.Additionally, jest's
toThrowErroris deprecated, in favor oftoThrow. I went ahead and made this replacement in every msal-common unit test file, where applicable.