chore - disposable tracker can differentiate between weak and strong leak

[jrieken]

No description provided.

[Tyriar]

Oh still failing too

[jrieken]

Oh still failing too

I know... There is plenty of bogous tests. Throwing the towel, it's not feasible.

@hediet @connor4312 I believe there is something fundamentally wrong with ensureNoDisposablesAreLeakedInTestSuite. Disposables like this case (which is a popular pattern) aren't tracked. When the store get's constructed there is no tracker yet. This PR wants to introduce the notion of a weak and hard leak, e.g. an empty disposable store that's leaked isn't as bad as leaked true disposable. This means the tracker is invoked multiple times for the same object (like on first add of the store) and that unveils all the ugly cases from above.

I am drafting this PR and give it to you, maybe there is something clever that can be done. Otherwise it isn't worth the effort and every leak is a bad leak

[Tyriar]

I believe there is something fundamentally wrong with ensureNoDisposablesAreLeakedInTestSuite. Disposables like this case (which is a popular pattern) aren't tracked.

This was always been how it's worked and was known to at least some people. Anything created before you call ensure will not be tracked, sometimes I think this was beneficial to people in adopting it since you could stick the ensure call after setup and not need to deal with making sure just test setup code was not being leaked (eg. store.add on a bunch of services). I can't remember if it was called it out, but I assumed it was known by most since they explicitly put the call after all the tests which is a weird place to put it for no reason.

If we want to "fix" this, I think we'd need another team-wide debt task with an eslint rule that forces the ensure call to be the first call inside suite so that it's setup and teardown always come first.

vscode/src/vs/workbench/contrib/terminal/test/browser/terminalInstance.test.ts

Lines 120 to 121 in 521e898

	suite('Workbench - TerminalInstance', () => {
	const store = ensureNoDisposablesAreLeakedInTestSuite();

For the case where you want a teardown to run before leaked disposables are detected you could then use store.add(toDisposable(myTeardown))

---

For the case you mentioned specifically, can't you just change that line to store = ensureNoDisposablesAreLeakedInTestSuite(); and remove the disposables.clear(); in teardown?

vscode/src/vs/workbench/services/label/test/browser/label.test.ts

Line 232 in afbaaa5

const disposables = new DisposableStore();