Linux build failure: Notebook Editor Integration Test

[joaomoreno]

https://dev.azure.com/vscode/VSCode/_build/results?buildId=77644&view=logs&j=a8a6689c-5a84-59c0-482a-7dd9f21eb2d8&t=1fa6fb8d-7bb3-5acd-5778-bf99aac7f637&l=682

  1) Notebook Editor
       Opening a notebook should fire activeNotebook event changed only once:
     Error: Unexpected type
      at assertType (vscode-file://vscode-app/mnt/vss/_work/1/s/out/vs/base/common/types.js:67:19)
      at SimpleNotebookEditorModel.load (vscode-file://vscode-app/mnt/vss/_work/1/s/out/vs/workbench/contrib/notebook/common/notebookEditorModel.js:457:35)
      at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
      at async NotebookModelReferenceCollection.createReferencedObject (vscode-file://vscode-app/mnt/vss/_work/1/s/out/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.js:64:26)
      at async NotebookModelResolverServiceImpl.resolve (vscode-file://vscode-app/mnt/vss/_work/1/s/out/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.js:184:31)
      at async MainThreadNotebookDocuments.$tryOpenNotebook (vscode-file://vscode-app/mnt/vss/_work/1/s/out/vs/workbench/api/browser/mainThreadNotebookDocuments.js:117:25)

[joaomoreno]

Another one: https://dev.azure.com/vscode/VSCode/_build/results?buildId=77639&view=logs&j=a8a6689c-5a84-59c0-482a-7dd9f21eb2d8&t=1fa6fb8d-7bb3-5acd-5778-bf99aac7f637&l=669

[joaomoreno]

Another one: https://dev.azure.com/vscode/VSCode/_build/results?buildId=77631&view=logs&j=a8a6689c-5a84-59c0-482a-7dd9f21eb2d8&t=1fa6fb8d-7bb3-5acd-5778-bf99aac7f637&l=681

[joaomoreno]

This is actually preventing a bunch of PRs to make it into main.

[joaomoreno]

Skipped this test: #165150

[joaomoreno]

Decided to comment out the entire test suite: #165150 (comment)

[rebornix]

Thanks for reverting it, this failure is what I was expecting to see but didn't realize it's so common.

Previously we often have NPE for SimpleNotebookEditorModel.notebook as after load, we guarantee the model is a IResolvedNotebookEditorModel. IResolvedNotebookEditorModel#notebook is not undefined, however our isResolved() check previously only checked for Boolean(this._workingCopy); instead of Boolean(this._workingCopy?.model?.notebookModel);. Not we fixed isResolved() condition, it starts to fail when we do the type assert

vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts

Lines 541 to 542 in 8b4642a

	assertType(this.isResolved());
	return this;

@jrieken can you help take a look at this?

[jrieken]

@bpasero This might actually be interesting for your. AFAIK we simply call resolve on the working copy manager but working copy actually isn't resolved. This is all happening here

vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts

Line 511 in 86127c6

async load(options?: INotebookLoadOptions): Promise<IResolvedNotebookEditorModel> {

The working copy doc says that the model is defined once resolve is called but it seems not so (at times)

[bpasero]

Maybe running with --verbose in that flaky test gives more info, we print a good amount of details for what is going on in the working copy when resolving or saving:

[bpasero]

#167212 brings the tests back and sets verbose logging for the failing one (I am not sure why the entire suite was disabled).

There is cases where manager.resolve can return early and maybe that happens here in this case. For example, when a disposed working copy is resolved:

vscode/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopy.ts

Lines 411 to 415 in 557a681

	if (this.isDisposed()) {
	this.trace('resolve() - exit - without resolving because file working copy is disposed');
	return;
	}

Or when providing contents for resolving but the working copy is dirty with user changes:

vscode/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopy.ts

Lines 420 to 424 in 557a681

	if (!options?.contents && (this.dirty || this.saveSequentializer.hasPending())) {
	this.trace('resolve() - exit - without resolving because file working copy is dirty or being saved');
	return;
	}

Anyway, any of these cases should appear in the logs now with verbose logging enabled.

[bpasero]

Does not seem to fail anymore