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.

Sign up for GitHub

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

Jump to bottom

Memory leak in tests: sinon.stub() without sinon.restore() #187471

Closed
12 tasks done
alexr00 opened this issue Jul 10, 2023 · 12 comments · Fixed by #187476
Closed
12 tasks done

Memory leak in tests: sinon.stub() without sinon.restore() #187471

alexr00 opened this issue Jul 10, 2023 · 12 comments · Fixed by #187476
Assignees
@rebornix @lramos15 @alexr00
Labels
debt Code quality issues important Issue identified as high-priority
Milestone
July 2023

Comments

@alexr00
Copy link
Member

alexr00 commented Jul 10, 2023 •
edited by rebornix

In short, if you use sinon.stub() without calling sinon.restore() you are leaking memory.

See sinonjs/sinon#1866 for the details.

I'm seeing a lot of memory used in unit tests and it appears to be coming from sinon. This may be contributing to our unit test OOM issue on Windows 32 bit. Let's see if fixing this lets us run the unit tests again.

To fix instatiationServiceMock.ts I have made TestInstantiationService disposable, and there were some places where tests failed or the refactor seemed larger:
@alexr00 alexr00 added the debt Code quality issues label Jul 10, 2023
@alexr00 alexr00 added this to the July 2023 milestone Jul 10, 2023
@alexr00
Copy link
Member Author

alexr00 commented Jul 10, 2023

The biggest gain would be from fixing the leak from instantionServiceMock.ts as it is used many places.

@alexr00
Copy link
Member Author

alexr00 commented Jul 10, 2023

@sandy081 I don't know if you own instantionServiceMock.ts, but I am looking into a fix for it since it will have the biggest impact.

@lramos15 lramos15 mentioned this issue Jul 10, 2023
Merged
@joaomoreno joaomoreno added the important Issue identified as high-priority label Jul 10, 2023
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Jul 10, 2023
@joaomoreno joaomoreno reopened this Jul 10, 2023
@VSCodeTriageBot VSCodeTriageBot removed the unreleased Patch has not yet been released in VS Code Insiders label Jul 10, 2023
@joaomoreno
Copy link
Member

joaomoreno commented Jul 10, 2023 •
edited

@lramos15, I've reopened this.

@alexr00 alexr00 mentioned this issue Jul 10, 2023
Merged
alexr00 added a commit that referenced this issue Jul 10, 2023
@alexr00
Dispose instances of test instantiation service
4910d9c 
This will call `sinon.restore()` and prevent a memory leak
Part of #187471
@lramos15
Copy link
Member

@lramos15, I've reopened this.

Thanks. I should be careful to not do Fixes X and instead Part of X as that will prevent GitHub from auto closing it

@alexr00
Copy link
Member Author

alexr00 commented Jul 10, 2023 •
edited

With #187482, I have made TestInstantiationService disposable and handled disposing it in most tests. There are several places where the way to handle this isn't clear or made test fail when I just naively disposed it. I have added them in the issue above.

@connor4312 connor4312 removed their assignment Jul 10, 2023
@connor4312 connor4312 mentioned this issue Jul 10, 2023
Merged
connor4312 added a commit that referenced this issue Jul 10, 2023
@connor4312
testing: fix memory leak during unit tests
11e5deb 
For #187471
@andreamah andreamah mentioned this issue Jul 10, 2023
Merged
@andreamah
Copy link
Contributor

andreamah commented Jul 10, 2023 •
edited

@alexr00 the instance in searchResult.test.ts is already restored at

vscode/src/vs/workbench/contrib/search/test/browser/searchResult.test.ts

Line 266 in fd63cf6

addContext.restore();
, but the others should be fixed in #187489. Let me know if I miss any!

Edit: I ended up making changes in both files.

connor4312 added a commit that referenced this issue Jul 10, 2023
@connor4312
testing: fix memory leak during unit tests (#187488)
84fc1ff 
For #187471
@andreamah andreamah removed their assignment Jul 10, 2023
alexr00 added a commit that referenced this issue Jul 11, 2023
@alexr00
Dispose instances of test instantiation service
67d35f8 
This will call `sinon.restore()` and prevent a memory leak
Part of #187471
@sandy081
Copy link
Member

@alexr00 For those tests where I am just using TestInstantiationService, should I have to still call sinon.restore() or I should wait for your change to dispose the TestInstantiationService ?

@sandy081 sandy081 mentioned this issue Jul 11, 2023
Merged
@alexr00
Copy link
Member Author

alexr00 commented Jul 11, 2023

You can wait for my change to dispose the TestInstantiationService.

alexr00 added a commit that referenced this issue Jul 11, 2023
@alexr00
Dispose instances of test instantiation service (#187482)
f0db763 
* Dispose instances of test instantiation service
This will call `sinon.restore()` and prevent a memory leak
Part of #187471

* Fix interfering terminal tests

* Remove `async` from terminal test suite

* Fix `any`
@sandy081 sandy081 mentioned this issue Jul 11, 2023
Merged
@sandy081 sandy081 removed their assignment Jul 11, 2023
Ulop pushed a commit to Megaputer/vscode that referenced this issue Jul 11, 2023
@alexr00
Dispose instances of test instantiation service (microsoft#187482)
5d13c1b 
* Dispose instances of test instantiation service
This will call `sinon.restore()` and prevent a memory leak
Part of microsoft#187471

* Fix interfering terminal tests

* Remove `async` from terminal test suite

* Fix `any`
@alexdima alexdima assigned rebornix and unassigned alexdima Jul 11, 2023
@joaomoreno
Copy link
Member

FYI Windows ia32 started turning green again 🥳 https://dev.azure.com/monacotools/Monaco/_build/results?buildId=222587&view=logs&j=306b2268-2a17-5e97-ab97-f41a26dc5206&t=55fb80e7-db05-5adc-8e66-0ba915c72c3e

@alexr00
Copy link
Member Author

alexr00 commented Jul 13, 2023

@lramos15 and @rebornix the only files left are yours. The TestInstantiationService is now disposable and should be disposed in these test files to properly restore sinon and prevent a memory leak.

@lramos15 lramos15 mentioned this issue Jul 13, 2023
Merged
rebornix added a commit that referenced this issue Jul 14, 2023
@rebornix
Re #187471. Dispose instantiationService on test finish.
9a76f9c
rebornix added a commit that referenced this issue Jul 14, 2023
@rebornix
Re #187471. Dispose instantiationService on test finish. (#187900)
43650e2
@alexr00
Copy link
Member Author

alexr00 commented Jul 14, 2023

👏 Thank you for the fast response everyone!

@alexr00 alexr00 closed this as completed Jul 14, 2023
@ghost

This comment was marked as off-topic.

Sign in to view
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rebornix rebornix

@lramos15 lramos15

@alexr00 alexr00

Labels
debt Code quality issues important Issue identified as high-priority
Projects
None yet
Milestone
July 2023
Development

Successfully merging a pull request may close this issue.

Restore sinon sandbox microsoft/vscode
9 participants
@joaomoreno @rebornix @connor4312 @lramos15 @alexdima @sandy081 @andreamah @alexr00 @VSCodeTriageBot