-
Notifications
You must be signed in to change notification settings - Fork 293
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
Submit tests for intellisense (replacing functional tests we had) #7565
Changes from 29 commits
e2f2fed
da73812
93b9b06
8709e6b
029a180
add4ede
9b49dab
75ef2bf
8a2b7fd
9e4b56a
c9801c2
b8be50b
fa76afe
7a20d2a
87cf668
d549f32
997320f
4123cf1
9d6f251
a63969d
852a557
4657383
1fc9570
a329aea
787ac1f
13149c8
5600040
c54ea19
6bc65f8
b830e93
2166345
23f5559
a93bf3a
19ecf09
9755d6c
0183f64
e3ef4a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ import { join } from 'path'; | |
import { | ||
Disposable, | ||
EventEmitter, | ||
ExtensionMode, | ||
languages, | ||
NotebookCell, | ||
NotebookCellKind, | ||
|
@@ -17,7 +16,7 @@ import { | |
Uri | ||
} from 'vscode'; | ||
import { ICommandManager, IDocumentManager, IVSCodeNotebook, IWorkspaceService } from '../../common/application/types'; | ||
import { isCI, JVSC_EXTENSION_ID, PYTHON_LANGUAGE } from '../../common/constants'; | ||
import { isCI, PYTHON_LANGUAGE } from '../../common/constants'; | ||
import { disposeAllDisposables } from '../../common/helpers'; | ||
import { traceInfo, traceInfoIf } from '../../common/logger'; | ||
import { | ||
|
@@ -49,7 +48,7 @@ import { | |
} from '../telemetry/telemetry'; | ||
import { KernelSocketInformation } from '../types'; | ||
import { NotebookCellLanguageService } from './cellLanguageService'; | ||
import { InteractiveWindowView, JupyterNotebookView } from './constants'; | ||
import { InteractiveWindowView } from './constants'; | ||
import { isJupyterNotebook, traceCellMessage, updateNotebookDocumentMetadata } from './helpers/helpers'; | ||
|
||
export class VSCodeNotebookController implements Disposable { | ||
|
@@ -86,7 +85,12 @@ export class VSCodeNotebookController implements Disposable { | |
public isAssociatedWithDocument(doc: NotebookDocument) { | ||
return this.associatedDocuments.has(doc); | ||
} | ||
|
||
public isPreferred(doc: NotebookDocument) { | ||
return this.preferredWith.has(doc); | ||
} | ||
private readonly associatedDocuments = new WeakSet<NotebookDocument>(); | ||
private readonly preferredWith = new WeakSet<NotebookDocument>(); | ||
constructor( | ||
private readonly kernelConnection: KernelConnectionMetadata, | ||
id: string, | ||
|
@@ -153,10 +157,10 @@ export class VSCodeNotebookController implements Disposable { | |
public async updateNotebookAffinity(notebook: NotebookDocument, affinity: NotebookControllerAffinity) { | ||
traceInfo(`Setting controller affinity for ${notebook.uri.toString()} ${this.id}`); | ||
this.controller.updateNotebookAffinity(notebook, affinity); | ||
// Only when running tests should we force the selection of the kernel. | ||
// Else the general VS Code behavior is for the user to select a kernel (here we make it look as though use selected it). | ||
if (this.context.extensionMode === ExtensionMode.Test) { | ||
await this.setAsActiveControllerForTests(notebook); | ||
|
||
// Keep track of if this controller is preferred for this notebook | ||
if (affinity === NotebookControllerAffinity.Preferred) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updateNotebookAffinity is called on each document open if you do something like this: Seems like that situation would be wrong then if I'm following right. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not following the mistake? Because A would still be preferred? Yeah seems like map should be the other way. Map between notebook (key) and controller (value) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, trying to make sure that I'm reading it right. As I see it now PreferredWith is all the notebook documents that this controller is the preferred controller for. Given that the preferred controller could change if you execute and reopen a document you could have the same document in the preferred list for two different controllers. As documents only have one preferred (at least from our extensions perspective) that situation wouldn't be right. |
||
this.preferredWith.add(notebook); | ||
} | ||
} | ||
|
||
|
@@ -178,6 +182,7 @@ export class VSCodeNotebookController implements Disposable { | |
await Promise.all(cells.map((cell) => this.executeCell(notebook, cell))); | ||
} | ||
private async onDidChangeSelectedNotebooks(event: { notebook: NotebookDocument; selected: boolean }) { | ||
traceInfoIf(isCI, `Notebook Controller base event called for ${this.id}. Selected ${event.selected} `); | ||
if (this.associatedDocuments.has(event.notebook) && event.selected) { | ||
// Possible it gets called again in our tests (due to hacks for testing purposes). | ||
return; | ||
|
@@ -408,31 +413,4 @@ export class VSCodeNotebookController implements Disposable { | |
await newKernel.start({ disableUI: true }).catch(noop); | ||
} | ||
} | ||
/** | ||
* In our tests, preferred controllers are setup as the active controller. | ||
* | ||
* This method is called on when running tests, else in the real world, | ||
* users need to select a kernel (preferred is on top of the list). | ||
*/ | ||
private async setAsActiveControllerForTests(notebook: NotebookDocument) { | ||
DonJayamanne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Only when running tests should we force the selection of the kernel. | ||
// Else the general VS Code behavior is for the user to select a kernel (here we make it look as though use selected it). | ||
if (this.context.extensionMode !== ExtensionMode.Test || notebook.notebookType !== JupyterNotebookView) { | ||
return; | ||
} | ||
traceInfoIf(isCI, `Command notebook.selectKernel executing for ${notebook.uri.toString()} ${this.id}`); | ||
await this.commandManager.executeCommand('notebook.selectKernel', { | ||
id: this.id, | ||
extension: JVSC_EXTENSION_ID | ||
}); | ||
traceInfoIf(isCI, `Command notebook.selectKernel exected for ${notebook.uri.toString()} ${this.id}`); | ||
// Used in tests to determine when the controller has been associated with a document. | ||
VSCodeNotebookController.kernelAssociatedWithDocument = true; | ||
|
||
// Sometimes the selection doesn't work (after all this is a hack). | ||
if (!this.associatedDocuments.has(notebook)) { | ||
this.associatedDocuments.add(notebook); | ||
this._onNotebookControllerSelected.fire({ notebook, controller: this }); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just turn this into
traceIfCI
, lol, feels kinda redundant to always pass inisCI
. What do you think.We might have a few places where we'd have some other conditions being passed in, but i think traceIfCI
or
traceOnCI` is simpler.Absolutely no need to make that change, just a thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds like a good idea. I'll add a debt issue