Skip to content

Enable intellisense and improve web smoke tests #9623

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

Merged
merged 2 commits into from
Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ jobs:
retention-days: 1

- name: Build web parts
run: npm run compile-web
run: npm run compile-web-test
if: always()

- name: Run web smoke test
Expand Down
33 changes: 33 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@
"order": 1
}
},
{
"name": "Extension (web with daemon compilation)",
"type": "pwa-extensionHost",
"debugWebWorkerHost": true,
"request": "launch",
"args": ["--extensionDevelopmentPath=${workspaceFolder}", "--extensionDevelopmentKind=web"],
"outFiles": ["${workspaceFolder}/out/**/*", "!${workspaceFolder}/**/node_modules**/*"],
"presentation": {
"group": "1_extension",
"order": 1
}
},
{
"name": "Extension (with daemon compilation)",
"type": "extensionHost",
Expand Down Expand Up @@ -88,6 +100,27 @@
"order": 3
}
},
{
"name": "Web Tests",
"type": "extensionHost",
"debugWebWorkerHost": true,
"request": "launch",
"args": [
"--extensionDevelopmentPath=${workspaceFolder}",
Copy link
Contributor Author

@DonJayamanne DonJayamanne Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this doesn't work because of node-fetch.
We can use cross-fetch (via webpack) to get around this, however i think its a bug caused by vscode.
node-fetch works in tests when run from the CLI, however when you debug it fails, something to do with global variables (in webworker, self is available, but when debugging self is not available, could be something vscode is shadowing or removing deliberately).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. a bug in vscode.

"--enable-proposed-api",
"--extensionDevelopmentKind=web",
"--extensionTestsPath=${workspaceFolder}/out/extension.web.bundle"
],
"outFiles": [
"${workspaceFolder}/out/**/*.*"
],
"sourceMaps": true,
"preLaunchTask": "compile-web-test",
"presentation": {
"group": "2_tests",
"order": 11
}
},
{
// Note, for the smoke test you want to debug, you may need to copy the file,
// rename it and remove a check for only smoke tests.
Expand Down
11 changes: 11 additions & 0 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@
},
"problemMatcher": ["$ts-webpack-watch"]
},
{
"label": "compile-web-test",
"type": "npm",
"script": "compile-web-test",
"isBackground": true,
"group": {
"kind": "build",
"isDefault": true
},
"problemMatcher": ["$ts-webpack-watch"]
},
{
"label": "Run Unit Tests",
"type": "npm",
Expand Down
19 changes: 19 additions & 0 deletions build/launchWebTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

const path = require('path');
const test_web = require('@vscode/test-web');
async function go() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, we don't need to compile the ts file anymore, we can just run webpack and that's it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides TS is smart enough to give decent completions for this.

try {
const extensionDevelopmentPath = path.resolve(__dirname, '../');
await test_web.runTests({
browserType: 'chromium',
extensionDevelopmentPath,
extensionTestsPath: path.join(extensionDevelopmentPath, 'out', 'extension.web.bundle')
});
} catch (err) {
console.error('Failed to run tests');
process.exit(1);
}
}
void go();
19 changes: 13 additions & 6 deletions build/webpack/webpack.extension.web.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,23 @@ const webpack = require('webpack');
const constants = require('../constants');
const CleanTerminalPlugin = require('clean-terminal-webpack-plugin');

const prodEntry = {
extension: './src/extension.web.ts',
'test/smoke.test/index': './src/test/web/smoke.test/index.ts' // source of the web extension test runner
};
const testEntry = {
extension: './src/test/web/smoke.test/index.ts' // source of the web extension test runner
};

// When running web tests, the entry point for the tests and extension are the same.
const entry = process.env.VSC_TEST_BUNDLE ? testEntry : prodEntry;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the magic happens


// tslint:disable-next-line:no-var-requires no-require-imports
const configFileName = path.join(constants.ExtensionRootDir, 'tsconfig.extension.web.json');
const config = {
mode: 'none',
mode: process.env.VSC_TEST_BUNDLE ? 'development' : 'none',
target: 'webworker',
entry: {
extension: './src/extension.web.ts',
'test/vscode.test/index': './src/test/web/vscode.test/index.ts', // source of the web extension test runner
'test/smoke.test/index': './src/test/web/smoke.test/index.ts' // source of the web extension test runner
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/vscode.test/index was never used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well not yet. I was going to use it to host the vscode tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's a vscode test?

},
entry,
devtool: 'nosources-source-map', // create a source map that points to the original source file
node: {
__dirname: false,
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2102,6 +2102,8 @@
"compile-webviews-watch": "webpack --config ./build/webpack/webpack.datascience-ui.config.js --watch",
"compile-web-watch": "webpack --config ./build/webpack/webpack.extension.web.config.js --stats-error-details --watch",
"compile-web": "webpack --config ./build/webpack/webpack.extension.web.config.js",
"compile-web-test": "cross-env VSC_TEST_BUNDLE=true npm run compile-web",
"compile-web-test-watch": "cross-env VSC_TEST_BUNDLE=true npm run compile-web-watch",
"compile-webviews-watchd": "deemon npm run compile-webviews-watch",
"compile-widgetTester": "cross-env NODE_OPTIONS=--max_old_space_size=9096 webpack --config ./build/webpack/webpack.datascience-ui-widgetTester.config.js --watch",
"kill-compile-webviews-watchd": "deemon --kill npm run compile-webviews-watch",
Expand All @@ -2125,7 +2127,7 @@
"testPerformance": "node ./out/test/testBootstrap.node.js ./out/test/performanceTest.js",
"testSmoke": "node ./out/test/testBootstrap.node.js ./out/test/smokeTest.node.js",
"testSmokeLogged": "cross-env VSC_JUPYTER_FORCE_LOGGING=true VSC_JUPYTER_LOG_FILE=smoke-test.log node --no-force-async-hooks-checks ./out/test/testBootstrap.node.js ./out/test/smokeTest.node.js",
"testSmokeWeb": "node ./out/test/smokeTest.web.js",
"testSmokeWeb": "node ./build/launchWebTest.js",
"lint": "eslint -c .eslintrc.js --ext .ts src",
"prettier-fix": "prettier 'src/**/*.ts*' --write && prettier 'build/**/*.js' --write",
"clean": "gulp clean",
Expand Down
9 changes: 0 additions & 9 deletions src/platform/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { INotebookEditorProvider } from '../notebooks/types';
import { IDataViewerDataProvider, IDataViewerFactory } from '../webviews/extension-side/dataviewer/types';
import { IExportedKernelService } from './api/extension';
import { IExportedKernelServiceFactory, IPythonApiProvider, PythonApi } from './api/types';
import { IApplicationShell } from './common/application/types';
import { isTestExecution } from './common/constants';
import { IExtensionContext } from './common/types';
import { IServiceContainer, IServiceManager } from './ioc/types';
Expand Down Expand Up @@ -102,14 +101,6 @@ export function buildApi(
/* eslint-disable @typescript-eslint/no-explicit-any */
(api as any).serviceContainer = serviceContainer;
(api as any).serviceManager = serviceManager;
(api as any).getSymbol = (symbol: String) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone now

// Needed for web tests. Eval for the extension happens in another web worker so
// the symbol objects have different values
switch (symbol) {
case 'IApplicationShell':
return IApplicationShell;
}
};
/* eslint-enable @typescript-eslint/no-explicit-any */
}
return api;
Expand Down
2 changes: 0 additions & 2 deletions src/test/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@
// Licensed under the MIT License.
'use strict';

import { interfaces } from 'inversify';
import { IExtensionApi } from '../platform/api';
import { IServiceContainer, IServiceManager } from '../platform/ioc/types';

export interface IExtensionTestApi extends IExtensionApi {
serviceContainer: IServiceContainer;
serviceManager: IServiceManager;
getSymbol<T>(symbolName: string): interfaces.ServiceIdentifier<T> | undefined;
}
25 changes: 0 additions & 25 deletions src/test/smokeTest.web.ts

This file was deleted.

81 changes: 58 additions & 23 deletions src/test/web/smoke.test/index.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,64 @@
// imports mocha for the browser, defining the `mocha` global.
require('mocha/mocha');
// Re-export extension entry point, so that the output from this file
// when bundled can be used as entry point for extension as well as tests.
// The same objects/types will be used as the module is only ever loaded once by nodejs.
import * as extension from '../../../extension.web';
import * as vscode from 'vscode';
import type { IExtensionApi } from '../../../platform/api';
import type { IExtensionContext } from '../../../platform/common/types';
import { IExtensionTestApi } from '../../common';
import { JVSC_EXTENSION_ID } from '../../../platform/common/constants';

export function run(): Promise<void> {
return new Promise((c, e) => {
mocha.setup({
ui: 'tdd',
reporter: undefined
});
let activatedResponse: undefined | IExtensionApi;

// bundles all files in the current directory matching `*.test`
const importAll = (r: __WebpackModuleApi.RequireContext) => r.keys().forEach(r);
importAll(require.context('.', true, /\.web.test$/));
// Basically this is the entry point for the extension.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bundle will be the entry point for the extension, and everything uses the same bundle and tests are in here too.

export async function activate(context: IExtensionContext): Promise<IExtensionApi> {
if (activatedResponse) {
return activatedResponse;
}
vscode.commands.registerCommand('jupyter.web.runTests', async () => {
// imports mocha for the browser, defining the `mocha` global.
require('mocha/mocha');

try {
// Run the mocha test
mocha.run((failures) => {
if (failures > 0) {
e(new Error(`${failures} tests failed.`));
} else {
c();
}
return new Promise<void>((resolve, reject) => {
mocha.setup({
ui: 'tdd',
reporter: undefined
});
} catch (err) {
console.error(err);
e(err);
}

// bundles all files in the current directory matching `*.test`
const importAll = (r: __WebpackModuleApi.RequireContext) => r.keys().forEach(r);
importAll(require.context('.', true, /\.web.test$/));

try {
// Run the mocha test
mocha.run((failures) => {
if (failures > 0) {
reject(new Error(`${failures} tests failed.`));
} else {
resolve();
}
});
} catch (err) {
console.error(err);
reject(err);
}
});
});
activatedResponse = await extension.activate(context);
return activatedResponse;
}

export async function deactivate(): Promise<void> {
return extension.deactivate();
}

export async function run(): Promise<void> {
// Activate the extension so that the commands are registered.
// Also this will not slow down the suite-setups.
const extension = vscode.extensions.getExtension<IExtensionTestApi>(JVSC_EXTENSION_ID)!;
const api = await extension.activate();
await api.ready;
// Run the tests from within the context of the extension bundle.
// We achieve this by getting the extension to run the tests (then its guaranteed to use the same context as the extension).
await vscode.commands.executeCommand('jupyter.web.runTests');
}
4 changes: 1 addition & 3 deletions src/test/web/smoke.test/smoke.web.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ suite('Web Extension Smoke Test Suite', () => {
});

test('Verify containers', () => {
const appShellSymbol = extensionApi.getSymbol<IApplicationShell>('IApplicationShell');
assert.ok(appShellSymbol, `Cannot get the symbol for IApplicationShell`);
const appShell = extensionApi.serviceManager?.get<IApplicationShell>(appShellSymbol!);
const appShell = extensionApi.serviceManager?.get<IApplicationShell>(IApplicationShell);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note; Previously we didn't get any intellisense in here at all, now we do.
This was because of an entry in tsconfig.json, now we get full code completion in these files.

assert.ok(appShell, 'Dependency Injection container not initialized in web context');
});
});
24 changes: 0 additions & 24 deletions src/test/web/vscode.test/extension.web.test.ts

This file was deleted.

29 changes: 0 additions & 29 deletions src/test/web/vscode.test/index.ts

This file was deleted.

3 changes: 1 addition & 2 deletions tsconfig.extension.web.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
"compilerOptions": {
"paths": {
"*": ["types/*"]
},
"types": ["@types/vscode-notebook-renderer/preload", "webpack-env"]
}
},
"exclude": [
"node_modules",
Expand Down
3 changes: 1 addition & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"resolveJsonModule": true,
"removeComments": true,
"useUnknownInCatchVariables": false,
"types": ["@types/vscode-notebook-renderer/preload"]
"types": ["@types/vscode-notebook-renderer/preload", "webpack-env"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better development experience (i.e. during editing we'll get code completions)

},
"exclude": [
"node_modules",
Expand All @@ -35,7 +35,6 @@
"src/ipywidgets",
"src/smoke",
"src/test/datascience/extensionapi",
"src/test/web",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better development experience (i.e. during editing we'll get code completions)

"build",
"out",
"ipywidgets",
Expand Down