-
Notifications
You must be signed in to change notification settings - Fork 294
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
Enable debugging IW in vscode.web and add tests for IW on web #10297
Conversation
.vscode/launch.json
Outdated
@@ -206,7 +206,7 @@ | |||
"VSC_JUPYTER_WEBVIEW_TEST_MIDDLEWARE": "true", // Initialize to create the webview test middleware | |||
"VSC_JUPYTER_LOAD_EXPERIMENTS_FROM_FILE": "true", | |||
// "TF_BUILD": "", // Set to anything to force full logging | |||
"TEST_FILES_SUFFIX": "vscode.test", | |||
"TEST_FILES_SUFFIX": "(*.vscode.test|*.vscode.common.test)", |
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.
Files with the extensions *.vscode.common.test will also run on the desktop.
@@ -265,13 +258,9 @@ export class CodeLensFactory implements ICodeLensFactory { | |||
Commands.RunCellAndAllBelowPalette | |||
]; | |||
} | |||
if (this.isWebExtension) { | |||
commandsToBeDisabled.push(Commands.DebugCell); |
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.
Debugging cells in IW web is now enabled.
@@ -78,7 +78,7 @@ export class DataScienceCodeLensProvider implements IDataScienceCodeLensProvider | |||
// CodeLensProvider interface | |||
// Some implementation based on DonJayamanne's jupyter extension work | |||
public provideCodeLenses(document: vscode.TextDocument, _token: vscode.CancellationToken): vscode.CodeLens[] { | |||
if (document.uri.scheme != PYTHON_FILE.scheme && document.uri.scheme !== PYTHON_UNTITLED.scheme) { | |||
if ([NotebookCellScheme, InteractiveInputScheme].includes(document.uri.scheme)) { |
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.
Running cells doesn't work in IW on the web unless you open a local folder, i.e. if you open a github repo or any other file system it will not work, hence now enabling it for all except notebook cells & IW input cell.
@@ -53,10 +53,7 @@ export class GlobalActivation implements IExtensionSingleActivationService { | |||
public async activate(): Promise<void> { | |||
if (this.dataScienceCodeLensProvider) { | |||
this.extensionContext.subscriptions.push( | |||
vscode.languages.registerCodeLensProvider( |
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.
Running cells doesn't work in IW on the web unless you open a local folder, i.e. if you open a github repo or any other file system it will not work, hence now enabling it for all except notebook cells & IW input cell.
} | ||
return { file: Uri.file(tempFile), dispose: () => swallowExceptions(() => fs.unlinkSync(tempFile)) }; | ||
}, | ||
async startJupyterServer(notebook?: NotebookDocument, useCert: boolean = false): Promise<any> { |
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.
We end up duplicating the test files because some functions like StartJupyter, createTempFile are specific to web vs desktop.
Created a common layer so that we call the common function and inject the actual implementation during runtime.
This way we don't need to duplicate files or the like. I.e. we can write some code that will run in all places (web & desktop tests)
export * from './helpers'; | ||
|
||
// The default base set of data science settings to use | ||
export function defaultDataScienceSettings(): IJupyterSettings { |
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.
All of these are common (can work on web as wel)
startJupyterServer: (notebook?: NotebookDocument) => Promise<void> | ||
) { | ||
suite.timeout(120_000); | ||
suite('DataScience - VSCode Notebook - Standard', function () { |
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.
A single test file for both web and desktop.
startJupyterServer: (notebook?: NotebookDocument) => Promise<void>; | ||
} | ||
) { | ||
suite('Kernel Event', function () { |
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.
Single test file for web & desktop
Codecov Report
@@ Coverage Diff @@
## main #10297 +/- ##
=====================================
- Coverage 64% 64% -1%
=====================================
Files 204 204
Lines 9295 9295
Branches 1505 1505
=====================================
- Hits 5963 5957 -6
- Misses 2864 2867 +3
- Partials 468 471 +3
|
startJupyterServer(notebook?: NotebookDocument, useCert?: boolean): Promise<void>; | ||
stopJupyterServer?(): Promise<void>; | ||
captureScreenShot?(fileNamePrefix: string): Promise<void>; | ||
initialize(): Promise<IExtensionTestApi>; |
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.
These are the functions that are different on each platform, and if we had a common function for them, then we won't have to create two 3 seprate files for the tests, now that we have a common layer for these functions, we have just one single file (tests that run on web & desktop)
startJupyterServer: (notebook?: NotebookDocument) => Promise<void>, | ||
handleTestFailure: (test: Mocha.Test) => Promise<void> | ||
) { | ||
suite('IPyWisdget Tests', function () { |
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.
Single file to test on web and desktop
} | ||
if (this.availableSecondPort) { | ||
await tcpPortUsed.waitUntilFree(this.availableSecondPort, 200, 5_000); | ||
await tcpPortUsed.waitUntilFree(this.availableSecondPort, 200, 5_000).catch(noop); |
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.
These unhandled exceptions were causing the test runner to report false positives.
test('Update variables during stepping', async () => { | ||
test('Update variables during stepping', async function () { | ||
if (isWeb()) { | ||
return this.skip(); |
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.
For some reason this isn't passing on the web, will fix seprately.
@@ -5,6 +5,22 @@ | |||
import * as vscode from 'vscode'; | |||
import { getFilePath } from '../../platform/common/platform/fs-paths'; | |||
import { traceInfo } from '../../platform/logging'; | |||
import { noop } from 'lodash'; |
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.
Moved all of this code from helpers.code.ts
import { sleep } from '../core'; | ||
import { IPYTHON_VERSION_CODE } from '../constants'; | ||
|
||
suite(`Interactive window`, async function () { |
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.
Interactive window tests for the web, very little work to enable this (once I enabled IW debugger tests for the web)
"testNativeNotebooksWithoutPythonInVSCode": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_JUPYTER_CI_TEST_VSC_CHANNEL=insiders TEST_FILES_SUFFIX=vscode.test* VSC_JUPYTER_FORCE_LOGGING=1 VSC_JUPYTER_LOAD_EXPERIMENTS_FROM_FILE=true VSC_JUPYTER_CI_TEST_GREP=non-python-kernel VSC_JUPYTER_CI_TEST_DO_NOT_INSTALL_PYTHON_EXT=true node ./out/test/testBootstrap.node.js ./out/test/standardTest.node.js", | ||
"testNativeNotebooksAndWebviews": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_JUPYTER_CI_TEST_VSC_CHANNEL=insiders TEST_FILES_SUFFIX=vscode.test* VSC_JUPYTER_FORCE_LOGGING=1 VSC_JUPYTER_CI_TEST_GREP=webview-test VSC_JUPYTER_LOAD_EXPERIMENTS_FROM_FILE=true node ./out/test/testBootstrap.node.js ./out/test/standardTest.node.js", | ||
"testNativeNotebooksWithoutPythonInVSCode": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_JUPYTER_CI_TEST_VSC_CHANNEL=insiders TEST_FILES_SUFFIX=*.vscode.test,*.vscode.common.test VSC_JUPYTER_FORCE_LOGGING=1 VSC_JUPYTER_LOAD_EXPERIMENTS_FROM_FILE=true VSC_JUPYTER_CI_TEST_GREP=non-python-kernel VSC_JUPYTER_CI_TEST_DO_NOT_INSTALL_PYTHON_EXT=true node ./out/test/testBootstrap.node.js ./out/test/standardTest.node.js", | ||
"testNativeNotebooksAndWebviews": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_JUPYTER_CI_TEST_VSC_CHANNEL=insiders TEST_FILES_SUFFIX=*.vscode.test,*.vscode.common.test VSC_JUPYTER_FORCE_LOGGING=1 VSC_JUPYTER_CI_TEST_GREP=webview-test VSC_JUPYTER_LOAD_EXPERIMENTS_FROM_FILE=true node ./out/test/testBootstrap.node.js ./out/test/standardTest.node.js", |
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.
New pattern to run *.vscode.test.ts and *.common.test.ts on desktop.
Unfortunately i cannot use a real glob patter like (*.vscode.test|*.common.test)
as this breaks the npm script. Using (
and |
in the npm scripts don't work.
@@ -221,7 +221,7 @@ export class KernelProcess implements IKernelProcess { | |||
tcpPortUsed.waitUntilUsed(this.connection.shell_port, 200, timeout), | |||
tcpPortUsed.waitUntilUsed(this.connection.iopub_port, 200, timeout) | |||
]).catch((ex) => { | |||
if (cancelToken.isCancellationRequested) { | |||
if (cancelToken.isCancellationRequested || deferred.rejected) { |
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.
No need to log errors if the other promise failed
This ensures we don't log errors unnecessarily (else we end up with false positives in the CI logs)
Uggh. We weren't going to submit this till I made the release branch. I'll just create the release branch from one before this. |
Then we have to cherry-pick Daniel's upcoming change. |
I think we need this for the release:
Changes/Fixes
Currently we only enable this for file schemes, i.e. if you open desktop files in web, meaning it will not work for other file systems like github or the like.
*.vscode.common.test.ts
that will run in both placesFixes #10296
Fixes #10107