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

Conversation

DonJayamanne
Copy link
Contributor

No description provided.

@DonJayamanne DonJayamanne requested a review from a team as a code owner April 7, 2022 18:02
"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.


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.

};

// 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

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?

@@ -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

// 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.

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.

@@ -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)

@@ -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)

@DonJayamanne DonJayamanne requested a review from rchiodo April 7, 2022 18:16
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #9623 (73f9748) into main (cae0758) will increase coverage by 0%.
The diff coverage is n/a.

@@         Coverage Diff          @@
##           main   #9623   +/-   ##
====================================
  Coverage    72%     72%           
====================================
  Files       193     194    +1     
  Lines      8451    8421   -30     
  Branches   1238    1229    -9     
====================================
- Hits       6136    6121   -15     
+ Misses     1851    1829   -22     
- Partials    464     471    +7     
Impacted Files Coverage Δ
src/platform/api.ts 57% <ø> (+<1%) ⬆️
src/platform/common/helpers.node.ts 60% <0%> (-15%) ⬇️
src/platform/common/platform/fs-paths.node.ts 75% <0%> (-7%) ⬇️
src/platform/common/configuration/service.base.ts 76% <0%> (ø)
...atform/common/application/encryptedStorage.node.ts 44% <0%> (ø)
src/platform/common/helpers.ts 100% <0%> (ø)
...rc/platform/common/dataScienceSurveyBanner.node.ts 69% <0%> (+3%) ⬆️
src/platform/common/platform/fs-paths.ts 96% <0%> (+29%) ⬆️

@DonJayamanne DonJayamanne merged commit cd787d2 into main Apr 7, 2022
@DonJayamanne DonJayamanne deleted the webSmokeTests branch April 7, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants