Faster ci tests #11867

DonJayamanne · 2022-11-02T02:50:32Z

Summary

1. The goal is to have a certain set of Jobs that MUST pass for PRs

These jobs will run the following tests:

Tests without any @ in name of the test
Tests with the text @mandatory in the tests,

2. Almost all tests have been grouped into individual groups

I've added the following tags to describe each group, some of the include

@mandatory
- Tests with these in their titles will always be executed in the mandatory job. expected to ALWAYS pass
- The idea behind this is to give the author of the PR confidence about their changes.
- E.g. if I were to update some code in README.md, then I don't need to worry about anythign at all, except for the mandatory test.
- I have included a few basic tests into this category, such as one debugger test, one IW test, one kernel test & the like.
- This way we don't need to run 20 kernel tests or 10 IW tests or 5 debugger test,
- @mandatory tests are analogous to smoke tests
@kernelCore
- Used to categorize tests for core functionality of kernels, starting & execution of kernels.
- If one were to make changes to Kernel code they can focus on the mandatory tests to pass as well as Kernel tests.
- I.e. I can ignore variable viewer, I can ignore debugger test, etc.
@iw - Interactive window
@debugger - Debugger tests
@webView - Web View tests, such as plot viewer
@lsp - Language services, such as code completion, hover provider, etc
@kernelPicker - Kernel Picker tests
@kernelInstall - Tests for installation of ipykernel, etc
@python - Testing actiavtion of python envs, conda, etc
@jupyter - Testing starting of Jupyter servers
@widgets - Widget tests
@export - Import/export tests

Todo:

Convert integration tests into unit tests where possible, I've converted quite a few that were unncessary
Remove unnecessary integration tests
Make the tests faster
Its upto each engineer to run their tests in different envionments, e.g. if we think we need to test Debugger in Web, then its upto the maintainer of that code to ensure we add a CI job for that particular environment in the matrix as follows:

          - jupyterConnection: remote
            python: python
            pythonVersion: '3.10'
            os: ubuntu-latest
            tags: '@debugger'

DonJayamanne · 2022-11-02T02:51:39Z

.github/workflows/build-test.yml

- jupyter: [raw, local]
- python: [nonConda, conda]
- pythonVersion: ['3.9', '3.10']
+ jupyter: [raw]


Its upto maintainers of the code to have different test matrices for each area
We only run all tests in zmq and run specific tests in remote, jupyter, conda, etc

codecov-commenter · 2022-11-02T03:09:50Z

Codecov Report

Merging #11867 (a7c5469) into main (7d63d42) will decrease coverage by 1%.
The diff coverage is 66%.

❗ Current head a7c5469 differs from pull request most recent head 1878b02. Consider uploading reports for the commit 1878b02 to get more accurate results

@@           Coverage Diff           @@
##            main   #11867    +/-   ##
=======================================
- Coverage     63%      61%    -2%     
=======================================
  Files        490      491     +1     
  Lines      34454    34541    +87     
  Branches    5590     5600    +10     
=======================================
- Hits       21752    21265   -487     
- Misses     10635    11249   +614     
+ Partials    2067     2027    -40

Impacted Files	Coverage Δ
src/platform/interpreter/contracts.ts	`100% <ø> (ø)`
src/platform/api/pythonApi.ts	`69% <52%> (-3%)`	⬇️
src/kernels/execution/helpers.ts	`62% <66%> (+1%)`	⬆️
...lPythonAndRelatedNonPythonKernelSpecFinder.node.ts	`71% <81%> (-4%)`	⬇️
src/notebooks/controllers/controllerLoader.ts	`92% <100%> (ø)`
...otebooks/debugger/controllers/restartController.ts	`10% <0%> (-75%)`	⬇️
...ebooks/debugger/controllers/runByLineController.ts	`13% <0%> (-68%)`	⬇️
src/notebooks/debugger/kernelDebugAdapter.ts	`20% <0%> (-65%)`	⬇️
src/notebooks/debugger/protocolParser.node.ts	`14% <0%> (-65%)`	⬇️
src/notebooks/debugger/debuggingManager.ts	`30% <0%> (-50%)`	⬇️
... and 72 more

DonJayamanne · 2022-11-03T00:12:01Z

src/kernels/raw/launcher/kernelLauncher.unit.test.ts

+import { KernelProcess } from './kernelProcess.node';
+import { PortAttributesProviders } from '../port/portAttributeProvider.node';
+
+suite('kernel Launcher', () => {


Converted an integration test into unit test
Plan is to do this more often where possible

DonJayamanne · 2022-11-03T00:12:37Z

src/platform/api/pythonApi.ts

- } else {
- const env = await api.environments.resolveEnvironment(pythonPathOrPythonId);
- return this.trackResolvedEnvironment(env, false);
+ traceWarning(


Our CI tests fail, as python env haven't loaded completely and in our test code we try to get details of virtual envs which Python hasn't yet discovered

DonJayamanne · 2022-11-03T00:12:50Z

src/test/datascience/data-viewing/showInDataViewerPythonInterpreter.vscode.test.ts

@@ -19,7 +19,7 @@ import { disposeAllDisposables } from '../../../platform/common/helpers';
 import { IShowDataViewerFromVariablePanel } from '../../../messageTypes';

 /* eslint-disable @typescript-eslint/no-explicit-any, no-invalid-this */
-suite('DataScience - VSCode Notebook - (DataViewer)', function () {
+suite('DataScience - VSCode Notebook - (DataViewer) @webview', function () {


Tagging all tests so we can run specific tests

DonJayamanne · 2022-11-03T02:25:20Z

build/webpack/webpack.extension.web.config.js

@@ -106,6 +106,9 @@ const config = {
 typeof process.env.IS_PRE_RELEASE_VERSION_OF_JUPYTER_EXTENSION === 'string'
 ? process.env.IS_PRE_RELEASE_VERSION_OF_JUPYTER_EXTENSION
 : 'true'
+ ),
+ VSC_JUPYTER_CI_TEST_GREP: JSON.stringify(
+ typeof process.env.VSC_JUPYTER_CI_TEST_GREP === 'string' ? process.env.VSC_JUPYTER_CI_TEST_GREP : ''


Required to get grep working for Mocha in web tests.
No way to pass env variables into Mocha, but to inject them when building.

DonJayamanne · 2022-11-03T02:25:50Z

src/test/datascience/data-viewing/dataViewer.unit.test.ts

@@ -22,7 +22,7 @@ import { JupyterVariableDataProvider } from '../../../webviews/extension-side/da
 import { IDataViewer, IDataViewerDataProvider } from '../../../webviews/extension-side/dataviewer/types';
 import { MockMemento } from '../../mocks/mementos';

-suite('DataScience - DataViewer', () => {
+suite('DataViewer', () => {


Removed all of the unencessary DataScience - prefixes

DonJayamanne · 2022-11-03T02:26:13Z

src/test/datascience/notebook/helper.ts

- }
+ const interpreter = isWeb()
+ ? undefined
+ : await waitForCondition(


Original code was incorrect, we cannot have interpreter in web.

DonJayamanne · 2022-11-03T02:26:28Z

src/test/datascience/notebook/kernelEvents.vscode.common.test.ts

@@ -38,6 +39,9 @@ suite('Kernel Event', function () {
 let previousDisableJupyterAutoStartValue: boolean;
 this.timeout(120_000);
 suiteSetup(async function () {
+ if (isWeb()) {
+ return this.skip();


Not required in web

DonJayamanne · 2022-11-03T02:54:32Z

build/webTestReporter.js

@@ -23,7 +23,7 @@ const logsDir = path.join(ExtensionRootDir, 'logs');

 async function captureScreenShot(name, res) {
 const screenshot = require('screenshot-desktop');
- fs.ensureDirSync(path.join(logsDir, name));
+ fs.ensureDirSync(logsDir);


DonJayamanne · 2022-11-03T02:56:11Z

src/test/datascience/notebook/helper.ts

+ const interpreter = await waitForCondition(
+ () => interpreterService.getActiveInterpreter(),
+ defaultNotebookTestTimeout,
+ 'Active Interpreter is undefined.2'


Changed to try and wait for Python extension to get the actiev interpreter.
Hacky work around for Python extnesion issue microsoft/vscode-python#20147
Doesn't seem to work always, but left this here.

Also added .2 suffix to identify where where things were failing, left this here

DonJayamanne · 2022-11-03T02:56:37Z

src/test/web/index.ts

 mocha.setup({
 ui: 'tdd',
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
- reporter: CustomReporter as any
+ reporter: CustomReporter as any,
+ grep


We can now run specific tests in VS Code web as well

rebornix · 2022-11-03T03:42:13Z

.github/workflows/build-test.yml

- pythonVersion: '3.9'
- webOrDesktop: 'web'
+ # Mandatory tests
+ - jupyterConnection: remote


@DonJayamanne can you give us a deep dive of how the include works with matrix? We now have 17 jobs and it's hard to know how each job is generated by reading the matrix, include and when clauses in the steps down below.

when clauses in the steps down below.

Can you give me an example if thsi

I'm referring to https:/microsoft/vscode-jupyter/pull/11867/files#diff-063ca77e012f959eba648db60e6868875fd1e70e5f5ac081193d848f8d61ef32R633 the if clauses. It's not easy to know how a job is generated (which makes it hard to troubleshoot locally)

Maybe we can also go through the new job list and each owner can check whether the prior steps are required for their tests and investigate how we can simplify each job.

Aah yes, unfortunately thats because we need different CLIs to run each test,
E.g. running web tests and running standard tests require different CLIs, see run: npm run testWebExtension

DonJayamanne commented Nov 2, 2022

View reviewed changes

DonJayamanne force-pushed the fasterCITests branch 14 times, most recently from 592e135 to 1f96e81 Compare November 2, 2022 23:25

DonJayamanne commented Nov 3, 2022

View reviewed changes

DonJayamanne force-pushed the fasterCITests branch from 1689f87 to fd75019 Compare November 3, 2022 00:40

DonJayamanne marked this pull request as ready for review November 3, 2022 02:24

DonJayamanne commented Nov 3, 2022

View reviewed changes

github-actions bot assigned DonJayamanne Nov 3, 2022

DonJayamanne commented Nov 3, 2022

View reviewed changes

rebornix reviewed Nov 3, 2022

View reviewed changes

DonJayamanne added 20 commits November 4, 2022 09:22

Include grep into bundle of web

3c139ed

more logging

cbb0d81

oops

3886da4

Oops

ed38ca3

fixes

1cbf415

oops

b97472f

Better retry

2705e77

Slider isn't mandatory

a90c2a9

oops

4504674

Misc

a8719a1

Fix screen capture on web

7376411

oops

efab7ea

Remove unnecessary tests

caaf472

remove logging

20bb565

Fix installation of ipywidgets

6f11907

Misc

6f72a22

Remove more integration tests

697f5b0

Fixes

fef2f83

Disable perf test in PRs

d84787b

Add missing env variable

8d0262d

DonJayamanne force-pushed the fasterCITests branch 4 times, most recently from b387076 to 8d0262d Compare November 3, 2022 22:30

DonJayamanne added 2 commits November 4, 2022 09:31

Hacky work around for mandatory CI job

a7c5469

remove unnecessary test

1878b02

DonJayamanne enabled auto-merge (squash) November 3, 2022 22:55

amunger approved these changes Nov 3, 2022

View reviewed changes

DonJayamanne merged commit 5ee34a0 into main Nov 3, 2022

DonJayamanne deleted the fasterCITests branch November 3, 2022 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster ci tests #11867

Faster ci tests #11867

DonJayamanne commented Nov 2, 2022 •

edited

Loading

DonJayamanne Nov 2, 2022

codecov-commenter commented Nov 2, 2022 •

edited

Loading

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

DonJayamanne Nov 3, 2022

rebornix Nov 3, 2022

DonJayamanne Nov 3, 2022

rebornix Nov 3, 2022

rebornix Nov 3, 2022

DonJayamanne Nov 3, 2022

Faster ci tests #11867

Faster ci tests #11867

Conversation

DonJayamanne commented Nov 2, 2022 • edited Loading

Summary

1. The goal is to have a certain set of Jobs that MUST pass for PRs

2. Almost all tests have been grouped into individual groups

Todo:

Choose a reason for hiding this comment

codecov-commenter commented Nov 2, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne commented Nov 2, 2022 •

edited

Loading

codecov-commenter commented Nov 2, 2022 •

edited

Loading