From 3ee2aba7d9ad5fc3caf6cdd238b70f4d9b9fbfb4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Oct 2025 20:56:30 +0000 Subject: [PATCH 1/4] Initial plan From 2794490ab9636db7b33112c4f585985b159c03dd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Oct 2025 21:03:58 +0000 Subject: [PATCH 2/4] Add cancellation support for dependency search progress Co-authored-by: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> --- src/managers/builtin/pipUtils.ts | 1 + src/test/managers/builtin/pipUtils.unit.test.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/managers/builtin/pipUtils.ts b/src/managers/builtin/pipUtils.ts index d221f5a8..4cb4549c 100644 --- a/src/managers/builtin/pipUtils.ts +++ b/src/managers/builtin/pipUtils.ts @@ -178,6 +178,7 @@ export async function getProjectInstallable( { location: ProgressLocation.Notification, title: VenvManagerStrings.searchingDependencies, + cancellable: true, }, async (_progress, token) => { const results: Uri[] = ( diff --git a/src/test/managers/builtin/pipUtils.unit.test.ts b/src/test/managers/builtin/pipUtils.unit.test.ts index 076596e8..d08755c2 100644 --- a/src/test/managers/builtin/pipUtils.unit.test.ts +++ b/src/test/managers/builtin/pipUtils.unit.test.ts @@ -199,4 +199,19 @@ suite('Pip Utils - getProjectInstallable', () => { assert.ok(firstResult.uri, 'Should have a URI'); assert.ok(firstResult.uri.fsPath.startsWith(workspacePath), 'Should be in workspace directory'); }); + + test('should show cancellable progress notification', async () => { + // Arrange: Mock findFiles to return empty results + findFilesStub.resolves([]); + + // Act: Call getProjectInstallable + const workspacePath = Uri.file('/test/path/root').fsPath; + const projects = [{ name: 'workspace', uri: Uri.file(workspacePath) }]; + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects); + + // Assert: Verify withProgress was called with cancellable option + assert.ok(withProgressStub.calledOnce, 'Should call withProgress once'); + const progressOptions = withProgressStub.firstCall.args[0] as ProgressOptions; + assert.strictEqual(progressOptions.cancellable, true, 'Progress should be cancellable'); + }); }); From 9caba3f9a5eb43ad9e3db64abb7b440ea32aee38 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Oct 2025 21:07:38 +0000 Subject: [PATCH 3/4] Add comprehensive cancellation test for dependency search Co-authored-by: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> --- .../managers/builtin/pipUtils.unit.test.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/test/managers/builtin/pipUtils.unit.test.ts b/src/test/managers/builtin/pipUtils.unit.test.ts index d08755c2..fb484ad2 100644 --- a/src/test/managers/builtin/pipUtils.unit.test.ts +++ b/src/test/managers/builtin/pipUtils.unit.test.ts @@ -214,4 +214,41 @@ suite('Pip Utils - getProjectInstallable', () => { const progressOptions = withProgressStub.firstCall.args[0] as ProgressOptions; assert.strictEqual(progressOptions.cancellable, true, 'Progress should be cancellable'); }); + + test('should handle cancellation during file search', async () => { + // Arrange: Create a cancellation token that is already cancelled + const cancelledToken: CancellationToken = { + isCancellationRequested: true, + onCancellationRequested: () => ({ dispose: () => {} }), + }; + + // Mock withProgress to immediately call the callback with the cancelled token + withProgressStub.callsFake( + async ( + _options: ProgressOptions, + callback: ( + progress: Progress<{ message?: string; increment?: number }>, + token: CancellationToken, + ) => Thenable, + ) => { + return await callback({} as Progress<{ message?: string; increment?: number }>, cancelledToken); + }, + ); + + // Mock findFiles to check that token is passed + findFilesStub.callsFake((_pattern: string, _exclude: string, _maxResults: number, token: CancellationToken) => { + // Verify the cancellation token is passed to findFiles + assert.strictEqual(token, cancelledToken, 'Cancellation token should be passed to findFiles'); + return Promise.resolve([]); + }); + + // Act: Call getProjectInstallable + const workspacePath = Uri.file('/test/path/root').fsPath; + const projects = [{ name: 'workspace', uri: Uri.file(workspacePath) }]; + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects); + + // Assert: Verify findFiles was called with the cancellation token + assert.ok(findFilesStub.called, 'findFiles should be called'); + assert.strictEqual(findFilesStub.callCount, 4, 'Should call findFiles 4 times for different patterns'); + }); }); From a4a586d2bcff6ad07992008d33bab569095ac5e7 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Wed, 10 Dec 2025 09:38:44 -0800 Subject: [PATCH 4/4] add some clarifications --- src/managers/builtin/pipUtils.ts | 42 +++++- src/managers/builtin/venvStepBasedFlow.ts | 2 + .../creators/autoFindProjects.unit.test.ts | 12 +- .../managers/builtin/pipUtils.unit.test.ts | 141 +++++++++++++++++- 4 files changed, 178 insertions(+), 19 deletions(-) diff --git a/src/managers/builtin/pipUtils.ts b/src/managers/builtin/pipUtils.ts index 4cb4549c..54527667 100644 --- a/src/managers/builtin/pipUtils.ts +++ b/src/managers/builtin/pipUtils.ts @@ -78,17 +78,19 @@ async function getCommonPackages(): Promise { } async function selectWorkspaceOrCommon( - installable: Installable[], + api: PythonEnvironmentApi, + projects: PythonProject[] | undefined, + hasWorkspaceDeps: boolean, common: Installable[], showSkipOption: boolean, installed: string[], ): Promise { - if (installable.length === 0 && common.length === 0) { + if (!hasWorkspaceDeps && common.length === 0) { return undefined; } const items: QuickPickItem[] = []; - if (installable.length > 0) { + if (hasWorkspaceDeps) { items.push({ label: PackageManagement.workspaceDependencies, description: PackageManagement.workspaceDependenciesDescription, @@ -124,6 +126,12 @@ async function selectWorkspaceOrCommon( if (selected && !Array.isArray(selected)) { try { if (selected.label === PackageManagement.workspaceDependencies) { + // Lazy load: only search for dependencies when user selects this option + const installable = await getProjectInstallable(api, projects); + if (installable.length === 0) { + // No dependencies found, return to picker + return selectWorkspaceOrCommon(api, projects, hasWorkspaceDeps, common, showSkipOption, installed); + } return await selectFromInstallableToInstall(installable, undefined, { showBackButton }); } else if (selected.label === PackageManagement.searchCommonPackages) { return await selectFromCommonPackagesToInstall(common, installed, undefined, { showBackButton }); @@ -136,7 +144,7 @@ async function selectWorkspaceOrCommon( // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (ex: any) { if (ex === QuickInputButtons.Back) { - return selectWorkspaceOrCommon(installable, common, showSkipOption, installed); + return selectWorkspaceOrCommon(api, projects, hasWorkspaceDeps, common, showSkipOption, installed); } } } @@ -155,14 +163,36 @@ export async function getWorkspacePackagesToInstall( environment?: PythonEnvironment, log?: LogOutputChannel, ): Promise { - const installable = (await getProjectInstallable(api, project)) ?? []; + // Quick check if any dependency files exist (don't load/parse them yet) + const hasWorkspaceDeps = await hasProjectDependencies(project); let common = await getCommonPackages(); let installed: string[] | undefined; if (environment) { installed = (await refreshPipPackages(environment, log, { showProgress: true }))?.map((pkg) => pkg.name); common = mergePackages(common, installed ?? []); } - return selectWorkspaceOrCommon(installable, common, !!options.showSkipOption, installed ?? []); + return selectWorkspaceOrCommon(api, project, hasWorkspaceDeps, common, !!options.showSkipOption, installed ?? []); +} + +/** + * Quickly check if any project dependency files exist. + * This is a fast check that doesn't parse files, used to determine whether to show the option. + */ +export async function hasProjectDependencies(projects?: PythonProject[]): Promise { + if (!projects || projects.length === 0) { + return false; + } + const exclude = '**/{.venv*,.git,.nox,.tox,.conda,site-packages,__pypackages__}/**'; + + // Quick check: just see if ANY files exist (maxResults=1 for performance) + const results = await Promise.all([ + findFiles('**/*requirements*.txt', exclude, 1), + findFiles('*requirements*.txt', exclude, 1), + findFiles('**/requirements/*.txt', exclude, 1), + findFiles('**/pyproject.toml', exclude, 1), + ]); + + return results.some((uris) => uris.length > 0); } export async function getProjectInstallable( diff --git a/src/managers/builtin/venvStepBasedFlow.ts b/src/managers/builtin/venvStepBasedFlow.ts index b679cd62..97f96129 100644 --- a/src/managers/builtin/venvStepBasedFlow.ts +++ b/src/managers/builtin/venvStepBasedFlow.ts @@ -203,6 +203,8 @@ async function enterEnvironmentName(state: VenvCreationState): Promise { let findFilesStub: sinon.SinonStub; diff --git a/src/test/managers/builtin/pipUtils.unit.test.ts b/src/test/managers/builtin/pipUtils.unit.test.ts index fb484ad2..49f8db0c 100644 --- a/src/test/managers/builtin/pipUtils.unit.test.ts +++ b/src/test/managers/builtin/pipUtils.unit.test.ts @@ -5,7 +5,7 @@ import { CancellationToken, Progress, ProgressOptions, Uri } from 'vscode'; import { PythonEnvironmentApi, PythonProject } from '../../../api'; import * as winapi from '../../../common/window.apis'; import * as wapi from '../../../common/workspace.apis'; -import { getProjectInstallable } from '../../../managers/builtin/pipUtils'; +import { getProjectInstallable, hasProjectDependencies } from '../../../managers/builtin/pipUtils'; suite('Pip Utils - getProjectInstallable', () => { let findFilesStub: sinon.SinonStub; @@ -216,13 +216,15 @@ suite('Pip Utils - getProjectInstallable', () => { }); test('should handle cancellation during file search', async () => { - // Arrange: Create a cancellation token that is already cancelled + // ARRANGE: Simulate a scenario where the user cancels the operation + // Step 1: Create a pre-cancelled token to simulate user clicking "Cancel" button const cancelledToken: CancellationToken = { isCancellationRequested: true, onCancellationRequested: () => ({ dispose: () => {} }), }; - // Mock withProgress to immediately call the callback with the cancelled token + // Step 2: Override withProgress stub to pass the cancelled token to the search callback + // This simulates the progress dialog being cancelled and the token being propagated withProgressStub.callsFake( async ( _options: ProgressOptions, @@ -231,24 +233,149 @@ suite('Pip Utils - getProjectInstallable', () => { token: CancellationToken, ) => Thenable, ) => { + // Execute the callback with the cancelled token (simulating cancellation during the operation) return await callback({} as Progress<{ message?: string; increment?: number }>, cancelledToken); }, ); - // Mock findFiles to check that token is passed + // Step 3: Mock findFiles to verify the cancelled token is properly passed through + // This ensures cancellation propagates from withProgress -> getProjectInstallable -> findFiles findFilesStub.callsFake((_pattern: string, _exclude: string, _maxResults: number, token: CancellationToken) => { - // Verify the cancellation token is passed to findFiles + // VERIFY: The same cancellation token should be passed to each findFiles call assert.strictEqual(token, cancelledToken, 'Cancellation token should be passed to findFiles'); return Promise.resolve([]); }); - // Act: Call getProjectInstallable + // ACT: Call the function under test const workspacePath = Uri.file('/test/path/root').fsPath; const projects = [{ name: 'workspace', uri: Uri.file(workspacePath) }]; await getProjectInstallable(mockApi as PythonEnvironmentApi, projects); - // Assert: Verify findFiles was called with the cancellation token + // ASSERT: Verify the cancellation token was passed to all file search operations + // Even though cancelled, the function should attempt all searches (they'll just return empty quickly) assert.ok(findFilesStub.called, 'findFiles should be called'); + // getProjectInstallable searches for dependencies using 4 different file patterns assert.strictEqual(findFilesStub.callCount, 4, 'Should call findFiles 4 times for different patterns'); }); }); + +suite('Pip Utils - hasProjectDependencies', () => { + let findFilesStub: sinon.SinonStub; + + setup(() => { + findFilesStub = sinon.stub(wapi, 'findFiles'); + }); + + teardown(() => { + sinon.restore(); + }); + + test('should return true when requirements.txt exists', async () => { + // Arrange: Mock findFiles to return a requirements file + findFilesStub.callsFake((pattern: string, _exclude: string, maxResults?: number) => { + // Verify maxResults=1 is used for performance (quick check) + assert.strictEqual(maxResults, 1, 'Should use maxResults=1 for quick check'); + + if (pattern === '*requirements*.txt') { + return Promise.resolve([Uri.file('/test/path/root/requirements.txt')]); + } + return Promise.resolve([]); + }); + + // Act + const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }]; + const result = await hasProjectDependencies(projects); + + // Assert + assert.strictEqual(result, true, 'Should return true when requirements files exist'); + }); + + test('should return true when pyproject.toml exists', async () => { + // Arrange: Mock findFiles to return pyproject.toml + findFilesStub.callsFake((pattern: string, _exclude: string, maxResults?: number) => { + assert.strictEqual(maxResults, 1, 'Should use maxResults=1 for quick check'); + + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file('/test/path/root/pyproject.toml')]); + } + return Promise.resolve([]); + }); + + // Act + const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }]; + const result = await hasProjectDependencies(projects); + + // Assert + assert.strictEqual(result, true, 'Should return true when pyproject.toml exists'); + }); + + test('should return false when no dependency files exist', async () => { + // Arrange: Mock findFiles to return empty arrays + findFilesStub.resolves([]); + + // Act + const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }]; + const result = await hasProjectDependencies(projects); + + // Assert + assert.strictEqual(result, false, 'Should return false when no dependency files exist'); + // Verify all 4 patterns were checked + assert.strictEqual(findFilesStub.callCount, 4, 'Should check all 4 file patterns'); + }); + + test('should return false when no projects provided', async () => { + // Act + const result = await hasProjectDependencies(undefined); + + // Assert + assert.strictEqual(result, false, 'Should return false when no projects provided'); + assert.ok(!findFilesStub.called, 'Should not call findFiles when no projects'); + }); + + test('should return false when empty projects array provided', async () => { + // Act + const result = await hasProjectDependencies([]); + + // Assert + assert.strictEqual(result, false, 'Should return false when empty projects array'); + assert.ok(!findFilesStub.called, 'Should not call findFiles when projects array is empty'); + }); + + test('should use maxResults=1 for all patterns for performance', async () => { + // Arrange: Track all maxResults values + const maxResultsUsed: (number | undefined)[] = []; + findFilesStub.callsFake((_pattern: string, _exclude: string, maxResults?: number) => { + maxResultsUsed.push(maxResults); + return Promise.resolve([]); + }); + + // Act + const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }]; + await hasProjectDependencies(projects); + + // Assert: All calls should use maxResults=1 for performance + assert.strictEqual(maxResultsUsed.length, 4, 'Should make 4 findFiles calls'); + maxResultsUsed.forEach((value, index) => { + assert.strictEqual(value, 1, `Call ${index + 1} should use maxResults=1`); + }); + }); + + test('should short-circuit when first pattern finds a file', async () => { + // Arrange: First pattern returns a result + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/*requirements*.txt') { + return Promise.resolve([Uri.file('/test/path/root/dev-requirements.txt')]); + } + return Promise.resolve([]); + }); + + // Act + const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }]; + const result = await hasProjectDependencies(projects); + + // Assert: Should still return true even though only first pattern matched + assert.strictEqual(result, true, 'Should return true when any pattern finds files'); + // Note: All 4 patterns are checked in parallel with Promise.all, so all 4 calls happen + assert.strictEqual(findFilesStub.callCount, 4, 'All patterns checked in parallel'); + }); +});