Skip to content

Fix venv tests for terminal activation #16747

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.

Sign up for GitHub

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
karrtikr merged 10 commits into from
Jul 22, 2021
Merged

Fix venv tests for terminal activation #16747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
86e448c
Unskip venv tests
Jul 21, 2021
3eead17
Run only venv tests
Jul 21, 2021
d249e17
Do not stub experiments service
Jul 21, 2021
836c3a4
Use new terminal setting to update shell
Jul 21, 2021
8587d1a
Initialize conda shell
Jul 21, 2021
f1f9e80
Use new terminal setting everywhere
Jul 21, 2021
fa20991
Run all tests
Jul 22, 2021
6ddc852
Use python.condaPath as conda binary if set
Jul 22, 2021
3913d27
Add/Fix tests for custom conda path
Jul 22, 2021
01da446
Use conda init everywhere
Jul 22, 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: 1 addition & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ jobs:
}
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaExecPath $condaExecPath
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaPath
& $condaExecPath init --all

# 2. For `interpreterLocatorService.testvirtualenvs.ts`

Expand Down
1 change: 1 addition & 0 deletions .github/workflows/nightly-coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ jobs:
}
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaExecPath $condaExecPath
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaPath
& $condaExecPath init --all

# 2. For `interpreterLocatorService.testvirtualenvs.ts`

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/pr-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ jobs:
}
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaExecPath $condaExecPath
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaPath
& $condaExecPath init --all

@karrtikr karrtikr Jul 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Initialize all shells for conda activation (including powershell), so conda activation tests work even when not using cmd. We do not want to rely on this too much though (conda 🤷‍♂️) so still try to use cmd in the tests.

@kimadeline kimadeline Jul 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does this addition need to be repeated in all the workflow files that have a Prepare conda for venv tests step?

@karrtikr karrtikr Jul 22, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point 😬


# 2. For `interpreterLocatorService.testvirtualenvs.ts`

Expand Down Expand Up @@ -545,6 +546,7 @@ jobs:
}
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaExecPath $condaExecPath
& $condaPythonPath ./build/ci/addEnvPath.py ${{env.PYTHON_VIRTUAL_ENVS_LOCATION}} condaPath
& $condaExecPath init --all

# 2. For `interpreterLocatorService.testvirtualenvs.ts`

Expand Down
7 changes: 6 additions & 1 deletion src/client/pythonEnvironments/common/environmentManagers/conda.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as fsapi from 'fs-extra';
import * as path from 'path';
import { traceVerbose } from '../../../common/logger';
import { getEnvironmentVariable, getOSType, getUserHomeDir, OSType } from '../../../common/utils/platform';
import { exec, pathExists, readFile } from '../externalDependencies';
import { exec, getPythonSetting, pathExists, readFile } from '../externalDependencies';

import { PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../base/info';
import { parseVersion } from '../../base/info/pythonVersion';
Expand Down Expand Up @@ -238,6 +238,11 @@ export class Conda {

// Produce a list of candidate binaries to be probed by exec'ing them.
async function* getCandidates() {
const customCondaPath = getPythonSetting<string>('condaPath');

@karrtikr karrtikr Jul 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Conda tests were only failing on Windows. One of the reasons was that conda binary was not picked up from python.condaPath, due to which we were calling getCandidatesFromRegistry among other stuff, which was slow and tests timed out.

if (customCondaPath && customCondaPath !== 'conda') {
// If user has specified a custom conda path, use it first.
yield customCondaPath;
}
// Check unqualified filename first, in case it's on PATH.
yield 'conda';
if (getOSType() === OSType.Windows) {
Expand Down
23 changes: 13 additions & 10 deletions ...est/common/terminals/environmentActivationProviders/terminalActivation.testvirtualenvs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@ import {
import { EXTENSION_ROOT_DIR_FOR_TESTS, TEST_TIMEOUT } from '../../../constants';
import { sleep } from '../../../core';
import { initialize, initializeTest } from '../../../initialize';
import * as ExperimentHelpers from '../../../../client/common/experiments/helpers';

suite.skip('Activation of Environments in Terminal', () => {
suite('Activation of Environments in Terminal', () => {
const file = path.join(
EXTENSION_ROOT_DIR_FOR_TESTS,
'src',
Expand Down Expand Up @@ -61,13 +60,12 @@ suite.skip('Activation of Environments in Terminal', () => {
let experiments: IExperimentService;
const sandbox = sinon.createSandbox();
suiteSetup(async () => {
sandbox.stub(ExperimentHelpers, 'inDiscoveryExperiment').resolves(false);

@karrtikr karrtikr Jul 22, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This fixed the other non-conda venv tests.

Initially, when in discovery experiment, the resolver was slow and hence the tests were timing out. That has been fixed with https://github.com/microsoft/vscode-python-internalbacklog/issues/283 and hence tests now pass upon removing this line.

envPaths = await fs.readJson(envsLocation);
terminalSettings = vscode.workspace.getConfiguration('terminal', vscode.workspace.workspaceFolders![0].uri);
pythonSettings = vscode.workspace.getConfiguration('python', vscode.workspace.workspaceFolders![0].uri);
defaultShell.Windows = terminalSettings.inspect('integrated.shell.windows').globalValue;
defaultShell.Linux = terminalSettings.inspect('integrated.shell.linux').globalValue;
await terminalSettings.update('integrated.shell.linux', '/bin/bash', vscode.ConfigurationTarget.Global);
defaultShell.Windows = terminalSettings.inspect('integrated.defaultProfile.windows').globalValue;
defaultShell.Linux = terminalSettings.inspect('integrated.defaultProfile.linux').globalValue;
await terminalSettings.update('integrated.defaultProfile.linux', 'bash', vscode.ConfigurationTarget.Global);
experiments = (await initialize()).serviceContainer.get<IExperimentService>(IExperimentService);
});

Expand Down Expand Up @@ -105,11 +103,15 @@ suite.skip('Activation of Environments in Terminal', () => {
vscode.ConfigurationTarget.WorkspaceFolder,
);
await terminalSettings.update(
'integrated.shell.windows',
'integrated.defaultProfile.windows',
defaultShell.Windows,
vscode.ConfigurationTarget.Global,
);
await terminalSettings.update('integrated.shell.linux', defaultShell.Linux, vscode.ConfigurationTarget.Global);
await terminalSettings.update(
'integrated.defaultProfile.linux',
defaultShell.Linux,
vscode.ConfigurationTarget.Global,
);
await pythonSettings.update('condaPath', undefined, vscode.ConfigurationTarget.Workspace);
if (experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
await resetGlobalInterpreterPathSetting();
Expand Down Expand Up @@ -189,9 +191,10 @@ suite.skip('Activation of Environments in Terminal', () => {
await testActivation(envPaths.virtualEnvPath);
});
test('Should activate with conda', async () => {
// Powershell does not work with conda by default, hence use cmd.
await terminalSettings.update(
'integrated.shell.windows',

@karrtikr karrtikr Jul 22, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VSCode deprecated the old setting, which is when the tests started failing as conda tests needs cmd to run, if powershell is not configured.

'C:\\Windows\\System32\\cmd.exe',
'integrated.defaultProfile.windows',
'Command Prompt',
vscode.ConfigurationTarget.Global,
);
await pythonSettings.update('condaPath', envPaths.condaExecPath, vscode.ConfigurationTarget.Workspace);
Expand Down
21 changes: 12 additions & 9 deletions src/test/pythonEnvironments/base/locators/composite/resolverUtils.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,21 @@ import {
import { resolveBasicEnv } from '../../../../../client/pythonEnvironments/base/locators/composite/resolverUtils';

suite('Resolver Utils', () => {
let getWorkspaceFolders: sinon.SinonStub;
setup(() => {
sinon.stub(externalDependencies, 'getPythonSetting').withArgs('condaPath').returns('conda');
getWorkspaceFolders = sinon.stub(externalDependencies, 'getWorkspaceFolders');
getWorkspaceFolders.returns([]);
});

teardown(() => {
sinon.restore();
});

suite('Pyenv', () => {
const testPyenvRoot = path.join(TEST_LAYOUT_ROOT, 'pyenvhome', '.pyenv');
const testPyenvVersionsDir = path.join(testPyenvRoot, 'versions');
setup(() => {
sinon.stub(externalDependencies, 'getWorkspaceFolders').returns([]);
sinon.stub(platformApis, 'getEnvironmentVariable').withArgs('PYENV_ROOT').returns(testPyenvRoot);
});

Expand Down Expand Up @@ -68,7 +78,6 @@ suite('Resolver Utils', () => {
const testStoreAppRoot = path.join(testLocalAppData, 'Microsoft', 'WindowsApps');

setup(() => {
sinon.stub(externalDependencies, 'getWorkspaceFolders').returns([]);
sinon.stub(platformApis, 'getEnvironmentVariable').withArgs('LOCALAPPDATA').returns(testLocalAppData);
});

Expand Down Expand Up @@ -199,10 +208,6 @@ suite('Resolver Utils', () => {
};
}

setup(() => {
sinon.stub(externalDependencies, 'getWorkspaceFolders').returns([]);
});

teardown(() => {
sinon.restore();
});
Expand Down Expand Up @@ -265,7 +270,7 @@ suite('Resolver Utils', () => {
suite('Simple envs', () => {
const testVirtualHomeDir = path.join(TEST_LAYOUT_ROOT, 'virtualhome');
setup(() => {
sinon.stub(externalDependencies, 'getWorkspaceFolders').returns([testVirtualHomeDir]);
getWorkspaceFolders.returns([testVirtualHomeDir]);
});

teardown(() => {
Expand Down Expand Up @@ -318,7 +323,6 @@ suite('Resolver Utils', () => {
const testPosixKnownPathsRoot = path.join(TEST_LAYOUT_ROOT, 'posixroot');
const testLocation3 = path.join(testPosixKnownPathsRoot, 'location3');
setup(() => {
sinon.stub(externalDependencies, 'getWorkspaceFolders').returns([]);
sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Linux);
});

Expand Down Expand Up @@ -514,7 +518,6 @@ suite('Resolver Utils', () => {
setup(async () => {
sinon.stub(winreg, 'readRegistryValues').callsFake(fakeRegistryValues);
sinon.stub(winreg, 'readRegistryKeys').callsFake(fakeRegistryKeys);
sinon.stub(externalDependencies, 'getWorkspaceFolders').returns([]);
sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Windows);
});

Expand Down
15 changes: 15 additions & 0 deletions src/test/pythonEnvironments/common/environmentManagers/conda.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,12 @@ suite('Conda and its environments are located correctly', () => {
};
}

let getPythonSetting: sinon.SinonStub;

setup(() => {
osType = platform.OSType.Unknown;
getPythonSetting = sinon.stub(externalDependencies, 'getPythonSetting');
getPythonSetting.withArgs('condaPath').returns('conda');
homeDir = undefined;
execPath = [];
files = {};
Expand Down Expand Up @@ -189,6 +193,17 @@ suite('Conda and its environments are located correctly', () => {
expect(conda).to.equal(undefined, 'conda should be missing');
});

test('Must find conda using `python.condaPath` setting and prefer it', async () => {
getPythonSetting.withArgs('condaPath').returns('condaPath/conda');

files = {
condaPath: {
conda: JSON.stringify(condaInfo('4.8.0')),
},
};
await expectConda('/condaPath/conda');
});

test('Must find conda on PATH, and prefer it', async () => {
osType = platform.OSType.Linux;
execPath = ['/bin'];
Expand Down