Adopt ensureNoDisposablesAreLeakedInTestSuite in unit tests
#190503
Comments
connor4312
added
debt
connor4312
assigned brettcannon, bpasero, amunger, alexdima, aeschli, chrmarti, bhavyaus, andreamah and aiday-mar
This fixes disposable leaks in the testing tests. These tests also encompass tree views. One big source of leaks I found was `Event.chain`: technically, every step in the chain is an IDisposable and needs to be disposed. However, this is very noisy and unergonomic. As an alternative, I introduce a very small `chain2` implementation which only requires disposing the resulting event listener, like an ordinary emitter. It doesn't support debouncing, though it could we if want it to; there was only a single usable of the chained `debounce` method in our codebase. For #190503
This fixes disposable leaks in the testing tests. These tests also encompass tree views. One big source of leaks I found was `Event.chain`: technically, every step in the chain is an IDisposable and needs to be disposed. However, this is very noisy and unergonomic. As an alternative, I introduce a very small `chain2` implementation which only requires disposing the resulting event listener, like an ordinary emitter. It doesn't support debouncing, though it could we if want it to; there was only a single usable of the chained `debounce` method in our codebase. For #190503
|
I included fixes for leaks in the ListWidget in my PR #192026 -- others are likely to hit that very quickly and may want to wait on that to get in |
|
@connor4312 I might need your help in #192112. Even though I think I setup the disposables correctly, I still get an error around here, when I run the entire suite: Could you have a look? |
|
I think I have found the issue, there were more things (services) not getting disposed. |
|
@connor4312 we could improve the experience by having a global setup/teardown. let tracker: DisposableTracker | undefined;
setup(function (this: import('mocha').Context) {
if ((this.currentTest?.parent?.title ?? '').indexOf('noLeaks') !== -1) {
tracker = new DisposableTracker();
setDisposableTracker(tracker);
}
});
suite('myTest.noLeaks', () => {
test(...); // Fails if test leaks
});We could even invert this: let tracker: DisposableTracker | undefined;
setup(function (this: import('mocha').Context) {
if ((this.currentTest?.parent?.title ?? '').indexOf('.allowLeaks') === -1) {
tracker = new DisposableTracker();
setDisposableTracker(tracker);
}
});
suite('myTest.allowLeaks', () => {
test(...); // doesn't fail if disposable leak
});
suite('myTest', () => {
test(...); // fails if disposables leak
});What do you think? |
|
If you find all test files that you touched but don't use the disposable tracker yet, do this in Power Shell: |
|
@hediet running the symbol |
This fixes disposable leaks in the testing tests. These tests also encompass tree views. One big source of leaks I found was `Event.chain`: technically, every step in the chain is an IDisposable and needs to be disposed. However, this is very noisy and unergonomic. As an alternative, I introduce a very small `chain2` implementation which only requires disposing the resulting event listener, like an ordinary emitter. It doesn't support debouncing, though it could we if want it to; there was only a single usable of the chained `debounce` method in our codebase. For #190503
|
@jrieken Moved |
#190503 adopt ensureNoDisposablesAreLeakedInTestSuite
…#192936) More adoption of `ensureNoDisposablesAreLeakedInTestSuite` (microsoft#190503)
|
The bulk of this is done so we can remove it from the plan next month, @andreamah let's close this out as high priority next week though |
|
Thanks all. I will open a followup issue for owners of specific tests who haven't adopted the disposable checker |
and others
Background
Changes in the order tests ran in caused spurious failures in a test that used
ensureNoDisposablesAreLeakedInTestSuite. This was almost certainly a result of leaky tests further up the chain, but it's very hard to figure out which test it is. We should seek to adoptensureNoDisposablesAreLeakedInTestSuitein the unit tests we own.Adoption
ensureNoDisposablesAreLeakedInTestSuiteis a utility that ensures that disposables created in a test are disposed of. This is very useful to find leaks in code you own.If you have a
setupandteardownblocks, make sure the function is called before the former and after the latter, like so:suite:The method also returns a
store: DisposableStoreyou can use for convenience to dispose instances within your tests, like so:It will then emit an error in the "after each" step if there are any dangling disposables. The error shown will have a stacktrace to where the leaked disposable was created.
Sometimes, it will not be the specific disposable that wasn't disposed of, but the thing that created it. You will want to walk up the displayed stack to find where the leak is.
ensureNoDisposablesAreLeakedInTestSuite. We can do a more efficient pass later as we uncomment the leak detector.Ask
We have a lot of tests! Please spend some time in the next debt week adding this function call to your tests, prioritizing ones that do more complex setups such as those involving code editors. It's not expected that we'll get every test in the first pass, and we can start calling out individual tests in future items.
Owner: @bpasero
Owner: @joaomoreno
Owner: @rebornix
Owner: @amunger
Owner: @jrieken
Owner: @aeschli
Owner: @alexdima
Owner: @hediet
Owner: @mjbvz
Owner: @aiday-mar
Owner: @sandy081
Owner: @Tyriar
Owner: @roblourens
Owner: @TylerLeonhardt
Owner: @connor4312
Owner: @lramos15
Owner: @alexr00
Owner: @andreamah
Owner: @meganrogge
Owner: @lszomoru
The text was updated successfully, but these errors were encountered: