Skip to content
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

Fix interpreter tests #7410

Merged
merged 49 commits into from
Sep 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
3d0e80b
Fix interpreter tests
DonJayamanne Sep 3, 2021
7f934e9
Fixes
DonJayamanne Sep 3, 2021
00c498a
Better logging
DonJayamanne Sep 3, 2021
5903973
Better logging
DonJayamanne Sep 3, 2021
22978c0
Fixes
DonJayamanne Sep 3, 2021
18e6df1
Fixes
DonJayamanne Sep 3, 2021
96f13b4
Better fixes
DonJayamanne Sep 3, 2021
65b185c
Added some logging for CI
DonJayamanne Sep 3, 2021
9574014
More fixes
DonJayamanne Sep 3, 2021
69e79dc
Fix tests
DonJayamanne Sep 3, 2021
611a58f
Oops
DonJayamanne Sep 3, 2021
4711e19
oops
DonJayamanne Sep 3, 2021
85fd924
Fix tests
DonJayamanne Sep 3, 2021
95d3394
Fix tests
DonJayamanne Sep 3, 2021
b9fd41b
Oops
DonJayamanne Sep 3, 2021
2a1109e
Fixes
DonJayamanne Sep 3, 2021
62ff3db
Fixes
DonJayamanne Sep 3, 2021
1ed7b40
Oops
DonJayamanne Sep 3, 2021
310521f
Fix tests
DonJayamanne Sep 3, 2021
cfc7144
Skip test in remote & non-raw
DonJayamanne Sep 3, 2021
dc2bb66
Skip coverage for functional tests
DonJayamanne Sep 3, 2021
4aa8c46
Oops
DonJayamanne Sep 3, 2021
86c8142
Misc
DonJayamanne Sep 3, 2021
fc21221
More tweeks
DonJayamanne Sep 3, 2021
15af1b0
More fixes
DonJayamanne Sep 3, 2021
946dde4
More fixes
DonJayamanne Sep 4, 2021
4947189
Added logging
DonJayamanne Sep 4, 2021
66ca21f
More fixes
DonJayamanne Sep 7, 2021
1030254
More logging
DonJayamanne Sep 7, 2021
9f87900
Ignore files
DonJayamanne Sep 7, 2021
8703957
Add more logging
DonJayamanne Sep 7, 2021
f73ab80
Logging
DonJayamanne Sep 7, 2021
6b8eb52
oops
DonJayamanne Sep 7, 2021
ed2354d
Some more logging
DonJayamanne Sep 7, 2021
49462a2
Add logging
DonJayamanne Sep 7, 2021
3a8e2a3
Fixes
DonJayamanne Sep 7, 2021
25bbee2
Some more logging
DonJayamanne Sep 7, 2021
2040893
More logging
DonJayamanne Sep 7, 2021
4252041
Activate python
DonJayamanne Sep 7, 2021
47ef368
More logging
DonJayamanne Sep 7, 2021
dbba5f3
Remove logging
DonJayamanne Sep 7, 2021
04ff288
More fixes
DonJayamanne Sep 7, 2021
002c4f0
Oops
DonJayamanne Sep 7, 2021
5e16ec2
Disable python extension experiments
DonJayamanne Sep 7, 2021
34295a2
Fix tests
DonJayamanne Sep 7, 2021
982e074
Oops
DonJayamanne Sep 7, 2021
6d940d5
Fix tests
DonJayamanne Sep 7, 2021
3429ffc
Fixes
DonJayamanne Sep 7, 2021
80eca1a
Fix functional tests
DonJayamanne Sep 7, 2021
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
1 change: 0 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,6 @@ module.exports = {
'src/client/datascience/jupyter/interpreter/jupyterInterpreterSelectionCommand.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterOldCacheStateStore.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterSelector.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts',
'src/client/datascience/jupyter/commandLineSelector.ts',
'src/client/datascience/jupyter/jupyterConnection.ts',
'src/client/datascience/jupyter/jupyterPasswordConnect.ts',
Expand Down
4 changes: 4 additions & 0 deletions .github/actions/create-venv-for-tests/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,17 @@ runs:

python -m venv .venvkernel
source .venvkernel/bin/activate
python --version
python -c "import sys;print(sys.executable)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useful, this is how i figured some of the paths were different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No harm in leaving this.

python -m pip install ipykernel
python -m ipykernel install --user --name .venvkernel --display-name .venvkernel
python -m pip uninstall jedi --yes
python -m pip install jedi==0.17.2

python -m venv .venvnokernel
source .venvnokernel/bin/activate
python --version
python -c "import sys;print(sys.executable)"
python -m pip install ipykernel
python -m ipykernel install --user --name .venvnokernel --display-name .venvnokernel
python -m pip uninstall jedi --yes
Expand Down
23 changes: 14 additions & 9 deletions .github/workflows/build-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,8 @@ jobs:
- name: Install Python Libs
if: env.conda_python == ''
run: |
python --version
python -c "import sys;print(sys.executable)"
python -m pip --disable-pip-version-check install -t ./pythonFiles/lib/python --no-cache-dir --implementation py --no-deps --upgrade -r ./requirements.txt
python -m pip --disable-pip-version-check install -r build/debugger-install-requirements.txt
python ./pythonFiles/install_debugpy.py
Expand Down Expand Up @@ -556,22 +558,23 @@ jobs:
check_name: Functional Test Report ${{matrix.os}}
if: steps.test_functional_group.outcome == 'failure' && failure()

# Mosts tests are commented, hence nothing runs, thus no coverage (if not commented, CI falls over as this code expects coverage files)
# Upload unit test coverage reports for later use in the "reports" job.
- name: Upload unit test coverage reports
uses: actions/upload-artifact@v2
if: "(success() || failure()) && !contains(github.ref, 'refs/heads/release')"
with:
name: ${{runner.os}}-${{env.COVERAGE_REPORTS}}
path: .nyc_output
retention-days: 1
# # Mosts tests are commented, hence nothing runs, thus no coverage (if not commented, CI falls over as this code expects coverage files)
# # Upload unit test coverage reports for later use in the "reports" job.
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 throws error, and functional atests (old style) are almost non-existent, hence don't see the need to break CI just for coverage.
Once we've fixed our functional tests we can re-vist this

# - name: Upload unit test coverage reports
# uses: actions/upload-artifact@v2
# if: "(success() || failure()) && !contains(github.ref, 'refs/heads/release')"
# with:
# name: ${{runner.os}}-${{env.COVERAGE_REPORTS}}
# path: .nyc_output
# retention-days: 1

vscodeTests:
name: VS Code Tests # These tests run with Python extension & real Jupyter
runs-on: ${{ matrix.os }}
if: github.repository == 'microsoft/vscode-jupyter'
env:
VSC_FORCE_REAL_JUPYTER: 1
VSC_PYTHON_FORCE_LOGGING: 1
VSC_JUPYTER_CI_RUN_NON_PYTHON_NB_TEST: 1
strategy:
fail-fast: false
Expand Down Expand Up @@ -734,6 +737,8 @@ jobs:
- name: Install Python Libs
if: matrix.python != 'conda' && matrix.python != 'noPython'
run: |
python --version
python -c "import sys;print(sys.executable)"
python -m pip --disable-pip-version-check install -t ./pythonFiles/lib/python --no-cache-dir --implementation py --no-deps --upgrade -r ./requirements.txt
python -m pip --disable-pip-version-check install -r build/debugger-install-requirements.txt
python ./pythonFiles/install_debugpy.py
Expand Down
30 changes: 29 additions & 1 deletion src/client/api/pythonApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
import { inject, injectable, named } from 'inversify';
import { CancellationToken, Disposable, Event, EventEmitter, Memento, Uri } from 'vscode';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../common/application/types';
import { isCI } from '../common/constants';
import { trackPackageInstalledIntoInterpreter } from '../common/installer/productInstaller';
import { ProductNames } from '../common/installer/productNames';
import { InterpreterUri } from '../common/installer/types';
import { traceInfo } from '../common/logger';
import {
GLOBAL_MEMENTO,
IDisposableRegistry,
Expand Down Expand Up @@ -313,6 +315,8 @@ export class InterpreterSelector implements IInterpreterSelector {
return this.apiProvider.getApi().then((api) => api.getSuggestions(resource));
}
}

const interpreterCacheForCI = new Map<string, PythonEnvironment[]>();
// eslint-disable-next-line max-classes-per-file
@injectable()
export class InterpreterService implements IInterpreterService {
Expand Down Expand Up @@ -345,7 +349,24 @@ export class InterpreterService implements IInterpreterService {
@captureTelemetry(Telemetry.InterpreterListingPerf)
public getInterpreters(resource?: Uri): Promise<PythonEnvironment[]> {
this.hookupOnDidChangeInterpreterEvent();
return this.apiProvider.getApi().then((api) => api.getInterpreters(resource));
const promise = this.apiProvider.getApi().then((api) => api.getInterpreters(resource));
if (isCI) {
promise
.then((items) => {
const current = interpreterCacheForCI.get(resource?.toString() || '');
const itemToStore = items;
if (
current &&
(itemToStore === current || JSON.stringify(itemToStore) === JSON.stringify(current))
) {
return;
}
interpreterCacheForCI.set(resource?.toString() || '', itemToStore);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like Python extension stuff doesn't get logged, this was crucial to figuring out what was going on.
I've enabled this just for CI.
We can remove this, i'm not fussed. I thought it would be useful

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've used the logs from Python extension a lot in the past, hence feel this is useful, but NOT necessary.

Can remove if anyone things we don't need this,

Copy link
Member

Choose a reason for hiding this comment

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

Don't see an issue with leaving it in myself.

traceInfo(`Interpreter list for ${resource?.toString()} is ${JSON.stringify(items)}`);
})
.catch(noop);
}
return promise;
}
private workspaceCachedActiveInterpreter = new Map<string, Promise<PythonEnvironment | undefined>>();
@captureTelemetry(Telemetry.ActiveInterpreterListingPerf)
Expand All @@ -364,6 +385,13 @@ export class InterpreterService implements IInterpreterService {
this.workspaceCachedActiveInterpreter.delete(workspaceId);
}
});
if (isCI) {
promise
.then((item) =>
traceInfo(`Active Interpreter in Python API for ${resource?.toString()} is ${item?.path}`)
)
.catch(noop);
}
}
}
return promise;
Expand Down
1 change: 0 additions & 1 deletion src/client/datascience/export/exportInterpreterFinder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export class ExportInterpreterFinder {

// If an interpreter was not passed in, work with the main jupyter interperter
const selectedJupyterInterpreter = await this.jupyterInterpreterService.getSelectedInterpreter();

if (selectedJupyterInterpreter) {
if (await this.checkNotebookInterpreter(selectedJupyterInterpreter)) {
return selectedJupyterInterpreter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ export class JupyterInterpreterService {
// Nothing saved found, so check our current interpreter
if (!interpreter) {
const currentInterpreter = await this.interpreterService.getActiveInterpreter(undefined);

if (currentInterpreter) {
// If the current active interpreter has everything installed already just use that
if (await this.interpreterConfiguration.areDependenciesInstalled(currentInterpreter, token)) {
Expand Down
Loading