Skip to content

Commit d660a2f

Browse files
committed
Address package manager review feedback
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5aba47cf-0b43-48b9-bc22-75e625da70e8
1 parent 8578f24 commit d660a2f

12 files changed

Lines changed: 207 additions & 117 deletions

File tree

‎.github/instructions/testing-workflow.instructions.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,7 @@ envConfig.inspect
603603
- Create shared mock helpers (e.g., `createMockLogOutputChannel()`) instead of duplicating mock setup across multiple test files (1)
604604
- Use `sinon.useFakeTimers()` with `clock.tickAsync()` instead of `await new Promise(resolve => setTimeout(resolve, ms))` for debounce/timeout handling - eliminates flakiness and speeds up tests significantly (1)
605605
- Always compile tests (`npm run compile-tests`) before running them after adding new test cases - test counts will be wrong if running against stale compiled output (1)
606+
- **Delete stale compiled test output after removing a test source file**: `compile-tests` does not remove the corresponding file under `out/`, so local unit or integration runs may execute a deleted test until that artifact is cleaned (1)
606607
- Never create "documentation tests" that just `assert.ok(true)` — if mocking limitations prevent testing, either test a different layer that IS mockable, or skip the test entirely with a clear explanation (1)
607608
- When stubbing vscode APIs in tests via wrapper modules (e.g., `workspaceApis`), the production code must also use those wrappers — sinon cannot stub properties directly on the vscode namespace like `workspace.workspaceFolders`, so both production and test code must reference the same stubbable wrapper functions (4)
608609
- **Before writing tests**, check if the function under test calls VS Code APIs directly (e.g., `commands.executeCommand`, `window.createTreeView`, `workspace.getConfiguration`). If so, FIRST update the production code to use wrapper functions from `src/common/*.apis.ts` (create the wrapper if it doesn't exist), THEN write tests that stub those wrappers. This prevents CI failures where sinon cannot stub the vscode namespace (4)

‎src/managers/base/commands/packageManagerCommand.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ export abstract class PackageManagerCommand {
2828
protected pythonExecutable: string;
2929
protected cwd?: string;
3030
protected log?: LogOutputChannel;
31-
protected timeout: number = 300000;
31+
protected timeout: number | undefined;
3232
protected config?: WorkspaceConfiguration;
3333

3434
constructor(options: CommandConstructorOptions) {

‎src/managers/builtin/commands/availableVersions.ts‎

Lines changed: 65 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,47 @@ import type { Pep440Version } from '@renovatebot/pep440';
22
import { AvailableVersionsCommand, type AvailableVersionsExecuteArgs } from '../../base/commands/index';
33
import { runPython, runUV } from '../helpers';
44

5-
export interface PipAvailableVersionsExecuteArgs extends AvailableVersionsExecuteArgs {
6-
useJson?: boolean;
5+
function parseVersionsJson(output: string, tool: 'pip' | 'uv'): string[] {
6+
const match = output.match(/{[\s\S]*}/);
7+
if (!match) {
8+
throw new Error(`Unable to find package version JSON in ${tool} output.`);
9+
}
10+
11+
const parsed: unknown = JSON.parse(match[0]);
12+
if (
13+
typeof parsed !== 'object' ||
14+
parsed === null ||
15+
!('versions' in parsed) ||
16+
!Array.isArray(parsed.versions) ||
17+
!parsed.versions.every((version) => typeof version === 'string')
18+
) {
19+
throw new Error(`Unexpected package version JSON from ${tool}.`);
20+
}
21+
22+
return parsed.versions;
723
}
824

925
/**
1026
* Pip available versions command.
11-
* Parsed command: `python -m pip index versions <package> [--json] --python-version <version>`
27+
* Parsed command: `python -m pip index versions <package> --json --disable-pip-version-check --python-version <version>`
1228
* Official documentation: https://pip.pypa.io/en/stable/cli/pip_index/
1329
*/
1430
export class PipAvailableVersionsCommand extends AvailableVersionsCommand {
15-
protected buildCommand(executeArgs: PipAvailableVersionsExecuteArgs): string[] {
16-
const args = ['-m', 'pip', 'index', 'versions', executeArgs.packageName];
17-
if (executeArgs.useJson !== false) {
18-
args.push('--json');
19-
}
20-
args.push('--python-version', executeArgs.pythonVersion);
21-
return args;
31+
protected buildCommand(executeArgs: AvailableVersionsExecuteArgs): string[] {
32+
return [
33+
'-m',
34+
'pip',
35+
'index',
36+
'versions',
37+
executeArgs.packageName,
38+
'--json',
39+
'--disable-pip-version-check',
40+
'--python-version',
41+
executeArgs.pythonVersion,
42+
];
2243
}
2344

24-
async execute(executeArgs: PipAvailableVersionsExecuteArgs): Promise<Pep440Version[]> {
45+
async execute(executeArgs: AvailableVersionsExecuteArgs): Promise<Pep440Version[]> {
2546
const output = await runPython(
2647
this.pythonExecutable,
2748
this.buildCommand(executeArgs),
@@ -30,25 +51,41 @@ export class PipAvailableVersionsCommand extends AvailableVersionsCommand {
3051
executeArgs.cancellationToken,
3152
this.timeout,
3253
);
33-
if (executeArgs.useJson === false) {
34-
const match = output.match(/^Available versions:\s*(.+)$/im);
35-
return this.parseVersions(match?.[1].split(',') ?? [], executeArgs.includePrerelease);
36-
}
54+
return this.parseVersions(parseVersionsJson(output, 'pip'), executeArgs.includePrerelease);
55+
}
56+
}
3757

38-
const match = output.match(/{[\s\S]*}/);
39-
if (!match) {
40-
return [];
41-
}
58+
/**
59+
* Pip available versions command for Pip 21.2 through 25.0, before JSON output was supported.
60+
*/
61+
export class PipAvailableVersionsTextCommand extends AvailableVersionsCommand {
62+
protected buildCommand(executeArgs: AvailableVersionsExecuteArgs): string[] {
63+
return [
64+
'-m',
65+
'pip',
66+
'index',
67+
'versions',
68+
executeArgs.packageName,
69+
'--disable-pip-version-check',
70+
'--python-version',
71+
executeArgs.pythonVersion,
72+
];
73+
}
4274

43-
try {
44-
const parsed = JSON.parse(match[0]) as { versions?: string[] };
45-
return this.parseVersions(
46-
Array.isArray(parsed.versions) ? parsed.versions : [],
47-
executeArgs.includePrerelease,
48-
);
49-
} catch {
50-
return [];
75+
async execute(executeArgs: AvailableVersionsExecuteArgs): Promise<Pep440Version[]> {
76+
const output = await runPython(
77+
this.pythonExecutable,
78+
this.buildCommand(executeArgs),
79+
undefined,
80+
this.log,
81+
executeArgs.cancellationToken,
82+
this.timeout,
83+
);
84+
const match = output.match(/^Available versions:\s*(.+)$/im);
85+
if (!match) {
86+
throw new Error('Unable to parse available package versions from pip output.');
5187
}
88+
return this.parseVersions(match[1].split(','), executeArgs.includePrerelease);
5289
}
5390
}
5491

@@ -80,19 +117,6 @@ export class UvAvailableVersionsCommand extends AvailableVersionsCommand {
80117
executeArgs.cancellationToken,
81118
this.timeout,
82119
);
83-
const match = output.match(/{[\s\S]*}/);
84-
if (!match) {
85-
return [];
86-
}
87-
88-
try {
89-
const parsed = JSON.parse(match[0]) as { versions?: string[] };
90-
return this.parseVersions(
91-
Array.isArray(parsed.versions) ? parsed.versions : [],
92-
executeArgs.includePrerelease,
93-
);
94-
} catch {
95-
return [];
96-
}
120+
return this.parseVersions(parseVersionsJson(output, 'uv'), executeArgs.includePrerelease);
97121
}
98122
}

‎src/managers/builtin/commands/index.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
export { PipAvailableVersionsCommand, UvAvailableVersionsCommand } from './availableVersions';
1+
export {
2+
PipAvailableVersionsCommand,
3+
PipAvailableVersionsTextCommand,
4+
UvAvailableVersionsCommand,
5+
} from './availableVersions';
26
export { PipInstallCommand, UvInstallCommand } from './install';
37
export { PipListCommand, UvListCommand } from './list';
48
export { PipListDirectNamesCommand, UvListDirectNamesCommand } from './listDirectNames';

‎src/managers/builtin/commands/listDirectNames.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@ import { runPython, runUV } from '../helpers';
44

55
/**
66
* Pip list direct names command.
7-
* Parsed command: `python -m pip list --format=json --not-required`
7+
* Parsed command: `python -m pip list --format=json --not-required --disable-pip-version-check`
88
* Official documentation: https://pip.pypa.io/en/stable/cli/pip_list/
99
*/
1010
export class PipListDirectNamesCommand extends ListDirectNamesCommand {
1111
protected buildCommand(): string[] {
12-
return ['-m', 'pip', 'list', '--format=json', '--not-required'];
12+
return ['-m', 'pip', 'list', '--format=json', '--not-required', '--disable-pip-version-check'];
1313
}
1414

1515
async execute(executeArgs?: BaseExecuteArgs): Promise<Set<string>> {

‎src/managers/builtin/pipPackageManager.ts‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import {
1111
MarkdownString,
1212
ProgressLocation,
1313
ThemeIcon,
14-
window,
1514
} from 'vscode';
1615
import {
1716
DidChangePackagesEventArgs,
@@ -24,13 +23,14 @@ import {
2423
PythonEnvironment,
2524
PythonEnvironmentApi,
2625
} from '../../api';
27-
import { withProgress } from '../../common/window.apis';
26+
import { showErrorMessage, withProgress } from '../../common/window.apis';
2827
import { CommandConstructorOptions } from '../base/commands/index';
2928
import { updatePackagesAndNotify } from '../common/packageChanges';
3029
import { parsePackageSpecs } from '../common/packageUtils';
3130
import { createPipOrUvCommand } from './commands/factory';
3231
import {
3332
PipAvailableVersionsCommand,
33+
PipAvailableVersionsTextCommand,
3434
PipInstallCommand,
3535
PipListCommand,
3636
PipListDirectNamesCommand,
@@ -145,7 +145,7 @@ export class PipPackageManager implements PackageManager, Disposable {
145145
this.log.error('Error managing packages', e);
146146
if (!options.runHeadless) {
147147
setImmediate(async () => {
148-
const result = await window.showErrorMessage('Error managing packages', 'View Output');
148+
const result = await showErrorMessage('Error managing packages', 'View Output');
149149
if (result === 'View Output') {
150150
this.log.show();
151151
}
@@ -175,7 +175,7 @@ export class PipPackageManager implements PackageManager, Disposable {
175175
}
176176

177177
async refresh(environment: PythonEnvironment): Promise<void> {
178-
await window.withProgress(
178+
await withProgress(
179179
{
180180
location: ProgressLocation.Window,
181181
title: 'Refreshing packages',
@@ -226,7 +226,7 @@ export class PipPackageManager implements PackageManager, Disposable {
226226
this.log.error('Error refreshing packages', error);
227227
if (showErrors) {
228228
setImmediate(async () => {
229-
const result = await window.showErrorMessage('Error refreshing packages', 'View Output');
229+
const result = await showErrorMessage('Error refreshing packages', 'View Output');
230230
if (result === 'View Output') {
231231
this.log.show();
232232
}
@@ -263,11 +263,11 @@ export class PipPackageManager implements PackageManager, Disposable {
263263
throw new Error(`Python executable is unavailable for environment: ${environment.envId.id}`);
264264
}
265265

266-
// Normalize versions like '3.13.1.final.0' (Python's sys.version_info format) to '3.13.1'
267-
// before parsing, since pep440 only accepts valid PEP 440 version strings.
268-
const versionMatch = (environment.version ?? '').match(/^(\d+(?:\.\d+)*)/);
269-
const normalizedVersion = versionMatch?.[1] ?? '';
270-
const baseVersion = parse(normalizedVersion)?.base_version;
266+
// Normalize versions like '3.13.1.final.0' (Python's sys.version_info format) to '3.13.1'
267+
// before parsing, since pep440 only accepts valid PEP 440 version strings.
268+
const versionMatch = (environment.version ?? '').match(/^(\d+(?:\.\d+)*)/);
269+
const normalizedVersion = versionMatch?.[1] ?? '';
270+
const baseVersion = parse(normalizedVersion)?.base_version;
271271
if (!baseVersion) {
272272
throw new Error(`Python version is unavailable for environment: ${environment.envId.id}`);
273273
}
@@ -280,7 +280,7 @@ export class PipPackageManager implements PackageManager, Disposable {
280280
UvAvailableVersionsCommand,
281281
);
282282

283-
// For pip < 21.2.0, check version first
283+
// For pip < 21.2.0, check version first.
284284
if (availableVersionsCmd instanceof PipAvailableVersionsCommand) {
285285
const pipVersion = await this.getVersion(environment);
286286
if (!pipVersion) {
@@ -291,12 +291,17 @@ export class PipPackageManager implements PackageManager, Disposable {
291291
`Package version lookup requires pip 21.2 or newer; the environment has pip ${pipVersion.public}.`,
292292
);
293293
}
294-
const versions = await availableVersionsCmd.execute({
295-
packageName,
296-
pythonVersion: baseVersion,
297-
useJson: compare(pipVersion.public, '25.1') >= 0,
298-
});
299-
return versions.sort((a, b) => compare(b.public, a.public));
294+
if (compare(pipVersion.public, '25.1') >= 0) {
295+
const versions = await availableVersionsCmd.execute({
296+
packageName,
297+
pythonVersion: baseVersion,
298+
});
299+
return versions.sort((a, b) => compare(b.public, a.public));
300+
}
301+
302+
const textCommand = new PipAvailableVersionsTextCommand({ pythonExecutable, log: this.log });
303+
const textVersions = await textCommand.execute({ packageName, pythonVersion: baseVersion });
304+
return textVersions.sort((a, b) => compare(b.public, a.public));
300305
}
301306

302307
const versions = await availableVersionsCmd.execute({

‎src/managers/builtin/venvUtils.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import { getWorkspacePersistentState } from '../../common/persistentState';
2525
import { EventNames } from '../../common/telemetry/constants';
2626
import { sendTelemetryEvent } from '../../common/telemetry/sender';
2727
import { normalizePath } from '../../common/utils/pathUtils';
28-
import { isTestExecution } from '../../common/utils/testing';
2928
import { getVenvPythonPath } from '../../common/utils/virtualEnvironment';
3029
import {
3130
showErrorMessage,
@@ -581,7 +580,6 @@ export async function removeVenv(
581580

582581
const confirmed =
583582
options?.runHeadless === true ||
584-
isTestExecution() ||
585583
(
586584
await showWarningMessage(
587585
l10n.t('Are you sure you want to remove {0}?', displayPath),

‎src/managers/conda/commands/availableVersions.ts‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,30 @@ export class CondaAvailableVersionsCommand extends AvailableVersionsCommand {
1919
executeArgs.cancellationToken,
2020
);
2121

22-
try {
23-
const parsed = JSON.parse(output);
24-
if (parsed && typeof parsed === 'object' && Array.isArray(parsed[executeArgs.packageName])) {
25-
const versions = (parsed[executeArgs.packageName] as Array<{ version?: string }>)
26-
.map((entry) => entry.version?.trim() ?? '')
27-
.filter((version) => !!version);
28-
return this.parseVersions(versions, executeArgs.includePrerelease);
29-
}
30-
return [];
31-
} catch {
32-
return [];
22+
const parsed: unknown = JSON.parse(output);
23+
if (
24+
typeof parsed !== 'object' ||
25+
parsed === null ||
26+
!(executeArgs.packageName in parsed)
27+
) {
28+
throw new Error(`Conda returned unexpected package version data for: ${executeArgs.packageName}`);
29+
}
30+
31+
const entries = (parsed as Record<string, unknown>)[executeArgs.packageName];
32+
if (!Array.isArray(entries)) {
33+
throw new Error(`Conda returned unexpected package version data for: ${executeArgs.packageName}`);
3334
}
35+
const versions = entries.map((entry) => {
36+
if (
37+
typeof entry !== 'object' ||
38+
entry === null ||
39+
!('version' in entry) ||
40+
typeof entry.version !== 'string'
41+
) {
42+
throw new Error(`Conda returned an invalid package version entry for: ${executeArgs.packageName}`);
43+
}
44+
return entry.version.trim();
45+
});
46+
return this.parseVersions(versions, executeArgs.includePrerelease);
3447
}
3548
}

0 commit comments

Comments
 (0)