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

eng: cleanup some leaks around around editors and workbenchInstantiationService #190623

Merged
merged 6 commits into from Aug 18, 2023

Conversation

connor4312
Copy link
Member

I added a disposable leak tracker to a test that used workbenchInstantiationService.
This fixes the baseline leaks and some extra leaks with that function.

Fixes some leaks I found while looking at #190444, by adding ensureNoDisposablesAreLeakedInTestSuite to the suite.

I don't have a dev setup for inline chat, so I have not tested this beyond running tests and verifying the fix
…ionService

I added a disposable leak tracker to a test that used `workbenchInstantiationService`.
This fixes the baseline leaks and some extra leaks with that function.
@connor4312 connor4312 self-assigned this Aug 16, 2023
@connor4312 connor4312 enabled auto-merge (squash) August 16, 2023 22:04
@VSCodeTriageBot VSCodeTriageBot added this to the August 2023 milestone Aug 16, 2023
DonJayamanne
DonJayamanne previously approved these changes Aug 16, 2023
@bpasero
Copy link
Member

bpasero commented Aug 17, 2023

@connor4312 this is very cool but should probably be reviewed by each respective component owner?

@bpasero bpasero self-requested a review August 17, 2023 09:52
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request changes (or explanation) because of the busy loop in test

@connor4312
Copy link
Member Author

cc other owners

@joaomoreno @hediet @alexdima @sbatten

please let me know if anything you own in this PR looks concerning

@connor4312 connor4312 merged commit 6336ee7 into main Aug 18, 2023
6 checks passed
@connor4312 connor4312 deleted the connor4312/reduce-test-leaks-1 branch August 18, 2023 15:04
@bpasero
Copy link
Member

bpasero commented Aug 18, 2023

🙅 This needs to be reopened until I had a chance to review.

connor4312 added a commit that referenced this pull request Aug 18, 2023
connor4312 added a commit that referenced this pull request Aug 18, 2023
connor4312 added a commit that referenced this pull request Aug 21, 2023
Re-applies #190623 which I merged before everyone had a chance to review.

Alex and Ben, you were the two people whose code this touches and didn't
review it in the original PR.
bpasero added a commit that referenced this pull request Aug 23, 2023
* eng: reapply test leak fixes

Re-applies #190623 which I merged before everyone had a chance to review.

Alex and Ben, you were the two people whose code this touches and didn't
review it in the original PR.

* 💄

* fix merge

* better handling of editor listeners

---------

Co-authored-by: Benjamin Pasero <benjamin.pasero@microsoft.com>
Krzysztof-Cieslak pushed a commit to githubnext/vscode that referenced this pull request Sep 18, 2023
…ionService (microsoft#190623)

* inline chat: fix leaks found in testing

Fixes some leaks I found while looking at microsoft#190444, by adding ensureNoDisposablesAreLeakedInTestSuite to the suite.

I don't have a dev setup for inline chat, so I have not tested this beyond running tests and verifying the fix

* eng: cleanup some leaks around around editors and workbenchInstantiationService

I added a disposable leak tracker to a test that used `workbenchInstantiationService`.
This fixes the baseline leaks and some extra leaks with that function.

* fix build

* rm too-eager leak checker

* remove forgotten busy loop
Krzysztof-Cieslak pushed a commit to githubnext/vscode that referenced this pull request Sep 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants