From 3a5cc108e7e28bcdf071783a1bab93b028d2f8cc Mon Sep 17 00:00:00 2001 From: Rahmat Eka <155574253+rhmatzeka@users.noreply.github.com> Date: Thu, 5 Nov 2026 15:18:50 +0700 Subject: [PATCH] Respect TestRunRequest.exclude when running tests Tests hidden by a Test Explorer filter (or otherwise excluded from the run request) were still started and run. Build the excluded set once, including descendants, and skip those items when expanding the request into test cases, for both the workspace and the project-based paths. If nothing is left after exclusions, don't start the adapter at all: an empty ID list makes pytest run every test. Test-ID deduplication is unchanged. Fixes #25742 Co-authored-by: Andrew Lavin --- .../common/projectTestExecution.ts | 21 ++- .../common/testItemUtilities.ts | 32 +++- .../testing/testController/controller.ts | 1 + .../testController/workspaceTestAdapter.ts | 16 +- .../common/projectTestExecution.unit.test.ts | 152 ++++++++++++++++++ .../testItemUtilities.unit.test.ts | 82 ++++++++++ .../workspaceTestAdapter.unit.test.ts | 76 ++++++++- 7 files changed, 368 insertions(+), 12 deletions(-) create mode 100644 src/test/testing/testController/testItemUtilities.unit.test.ts diff --git a/src/client/testing/testController/common/projectTestExecution.ts b/src/client/testing/testController/common/projectTestExecution.ts index 8d4f62b508ac..46f7d682c7ea 100644 --- a/src/client/testing/testController/common/projectTestExecution.ts +++ b/src/client/testing/testController/common/projectTestExecution.ts @@ -14,6 +14,7 @@ import { TestProjectRegistry } from './testProjectRegistry'; import { getProjectId } from './projectUtils'; import { getEnvExtApi, useEnvExtension } from '../../../envExt/api.internal'; import { isParentPath } from '../../../pythonEnvironments/common/externalDependencies'; +import { expandExcludeSet } from './testItemUtilities'; /** Dependencies for project-based test execution. */ export interface ProjectExecutionDependencies { @@ -53,6 +54,9 @@ export async function executeTestsForProjects( setupCoverageForProjects(request, projects); } + // Excluded items and their descendants, built once for all projects + const excludeSet = expandExcludeSet(request.exclude); + // Execute tests for each project in parallel // For debug mode, multiple debug sessions will be launched in parallel // Each execution respects cancellation via runInstance.token @@ -76,7 +80,7 @@ export async function executeTestsForProjects( let failed = false; let failureCategory: UnitTestRunFailureCategory | undefined; try { - await executeTestsForProject(project, items, runInstance, request, deps); + await executeTestsForProject(project, items, runInstance, request, deps, excludeSet); } catch (error) { failed = true; failureCategory = token.isCancellationRequested ? 'cancelled' : 'unknown'; @@ -235,13 +239,14 @@ export async function executeTestsForProject( runInstance: TestRun, request: TestRunRequest, deps: ProjectExecutionDependencies, + excludeSet?: Set, ): Promise { const processedTestItemIds = new Set(); const uniqueTestCaseIds = new Set(); // Mark items as started and collect test IDs (deduplicated to handle overlapping selections) for (const item of testItems) { - const testCaseNodes = getTestCaseNodesRecursive(item); + const testCaseNodes = getTestCaseNodesRecursive(item, excludeSet); for (const node of testCaseNodes) { if (processedTestItemIds.has(node.id)) { continue; @@ -277,16 +282,22 @@ export async function executeTestsForProject( ); } -/** Recursively gets all leaf test case nodes from a test item tree. */ -export function getTestCaseNodesRecursive(item: TestItem): TestItem[] { +/** + * Recursively gets all leaf test case nodes from a test item tree, + * skipping items in `excludeSet` (built by `expandExcludeSet`). + */ +export function getTestCaseNodesRecursive(item: TestItem, excludeSet?: Set): TestItem[] { const results: TestItem[] = []; + if (excludeSet?.has(item)) { + return results; + } if (item.children.size === 0) { // This is a leaf node (test case) results.push(item); } else { // Recursively get children item.children.forEach((child) => { - results.push(...getTestCaseNodesRecursive(child)); + results.push(...getTestCaseNodesRecursive(child, excludeSet)); }); } return results; diff --git a/src/client/testing/testController/common/testItemUtilities.ts b/src/client/testing/testController/common/testItemUtilities.ts index 43624bba2527..bae42a654a2d 100644 --- a/src/client/testing/testController/common/testItemUtilities.ts +++ b/src/client/testing/testController/common/testItemUtilities.ts @@ -498,14 +498,42 @@ export async function updateTestItemFromRawData( item.busy = false; } -export function getTestCaseNodes(testNode: TestItem, collection: TestItem[] = []): TestItem[] { +/** + * Builds the set of test items excluded from a run: the items in `TestRunRequest.exclude` + * plus all of their descendants, so a single set lookup tells whether a node is excluded. + */ +export function expandExcludeSet(excludes: readonly TestItem[] | undefined): Set | undefined { + if (!excludes || excludes.length === 0) { + return undefined; + } + const expanded = new Set(); + const add = (item: TestItem) => { + if (expanded.has(item)) { + return; + } + expanded.add(item); + item.children.forEach(add); + }; + excludes.forEach(add); + return expanded; +} + +export function getTestCaseNodes( + testNode: TestItem, + collection: TestItem[] = [], + excludeSet?: Set, +): TestItem[] { + if (excludeSet?.has(testNode)) { + return collection; + } + if (!testNode.canResolveChildren && testNode.tags.length > 0) { collection.push(testNode); } testNode.children.forEach((c) => { if (testNode.canResolveChildren) { - getTestCaseNodes(c, collection); + getTestCaseNodes(c, collection, excludeSet); } else { collection.push(testNode); } diff --git a/src/client/testing/testController/controller.ts b/src/client/testing/testController/controller.ts index cd7b516af2b7..255d9a306d07 100644 --- a/src/client/testing/testController/controller.ts +++ b/src/client/testing/testController/controller.ts @@ -1052,6 +1052,7 @@ export class PythonTestController implements ITestController, IExtensionSingleAc request.profile?.kind, this.debugLauncher, await this.interpreterService.getActiveInterpreter(workspace.uri), + request.exclude, ); } catch (ex) { failed = true; diff --git a/src/client/testing/testController/workspaceTestAdapter.ts b/src/client/testing/testController/workspaceTestAdapter.ts index f81a5b4a7da7..b79077606646 100644 --- a/src/client/testing/testController/workspaceTestAdapter.ts +++ b/src/client/testing/testController/workspaceTestAdapter.ts @@ -6,11 +6,11 @@ import { CancellationToken, TestController, TestItem, TestRun, TestRunProfileKin import { createDeferred, Deferred } from '../../common/utils/async'; import { Testing } from '../../common/utils/localize'; import { StopWatch } from '../../common/utils/stopWatch'; -import { traceError } from '../../logging'; +import { traceError, traceVerbose } from '../../logging'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; import { TestProvider } from '../types'; -import { createErrorTestItem, getTestCaseNodes } from './common/testItemUtilities'; +import { createErrorTestItem, expandExcludeSet, getTestCaseNodes } from './common/testItemUtilities'; import { ITestDiscoveryAdapter, ITestExecutionAdapter, ITestResultResolver } from './common/types'; import { IPythonExecutionFactory } from '../../common/process/types'; import { ITestDebugLauncher } from '../common/types'; @@ -51,6 +51,7 @@ export class WorkspaceTestAdapter { profileKind?: boolean | TestRunProfileKind, debugLauncher?: ITestDebugLauncher, interpreter?: PythonEnvironment, + excludes?: readonly TestItem[], project?: ProjectAdapter, ): Promise { if (this.executing) { @@ -63,10 +64,11 @@ export class WorkspaceTestAdapter { const testCaseNodes: TestItem[] = []; const testCaseIdsSet = new Set(); + const excludeSet = expandExcludeSet(excludes); try { - // first fetch all the individual test Items that we necessarily want + // first fetch all the individual test Items that we necessarily want, minus the excluded ones includes.forEach((t) => { - const nodes = getTestCaseNodes(t); + const nodes = getTestCaseNodes(t, [], excludeSet); testCaseNodes.push(...nodes); }); // iterate through testItems nodes and fetch their unittest runID to pass in as argument @@ -78,6 +80,12 @@ export class WorkspaceTestAdapter { } }); const testCaseIds = Array.from(testCaseIdsSet); + if (excludeSet && testCaseIds.length === 0) { + // Everything was excluded. Don't start a run: an empty ID list makes pytest run all tests. + traceVerbose('All requested tests were excluded, nothing to run.'); + deferred.resolve(); + return deferred.promise; + } if (executionFactory === undefined) { throw new Error('Execution factory is required for test execution'); } diff --git a/src/test/testing/testController/common/projectTestExecution.unit.test.ts b/src/test/testing/testController/common/projectTestExecution.unit.test.ts index 2fe71e3a0697..07786cb31d40 100644 --- a/src/test/testing/testController/common/projectTestExecution.unit.test.ts +++ b/src/test/testing/testController/common/projectTestExecution.unit.test.ts @@ -28,6 +28,7 @@ import { groupTestItemsByProject, setupCoverageForProjects, } from '../../../../client/testing/testController/common/projectTestExecution'; +import { expandExcludeSet } from '../../../../client/testing/testController/common/testItemUtilities'; import * as telemetry from '../../../../client/telemetry'; import { EventName } from '../../../../client/telemetry/constants'; import * as envExtApi from '../../../../client/envExt/api.internal'; @@ -340,6 +341,21 @@ suite('Project Test Execution', () => { expect(result).to.have.length(3); expect(new Set(result)).to.deep.equal(new Set([leaf1, leaf2, leaf3])); }); + + test('should skip excluded items and their descendants', () => { + // Mock - file with two classes, one class excluded + const leaf1 = createMockTestItem('test1', '/test.py'); + const leaf2 = createMockTestItem('test2', '/test.py'); + const class1 = createMockTestItem('Class1', '/test.py', [leaf1]); + const class2 = createMockTestItem('Class2', '/test.py', [leaf2]); + const fileItem = createMockTestItem('test_file.py', '/test.py', [class1, class2]); + + // Run + const result = getTestCaseNodesRecursive(fileItem, expandExcludeSet([class2])); + + // Assert + expect(result).to.deep.equal([leaf1]); + }); }); // ===== executeTestsForProject Tests ===== @@ -439,6 +455,90 @@ suite('Project Test Execution', () => { const passedTestIds = project.executionAdapterStub.firstCall.args[1] as string[]; expect(passedTestIds).to.have.length(2); }); + + test('should not run test items in the exclude set', async () => { + // Mock - class containing two test methods, one excluded + const project = createMockProjectAdapter({ projectPath: '/workspace/proj', projectName: 'proj' }); + const leaf1 = createMockTestItem('test1', '/workspace/proj/test.py'); + const leaf2 = createMockTestItem('test2', '/workspace/proj/test.py'); + const classItem = createMockTestItem('TestClass', '/workspace/proj/test.py', [leaf1, leaf2]); + project.resultResolver.vsIdToRunId.set('test1', 'runId1'); + project.resultResolver.vsIdToRunId.set('test2', 'runId2'); + const runMock = createMockTestRun(); + const request = { profile: { kind: TestRunProfileKind.Run } } as TestRunRequest; + const deps = createMockDependencies(); + + // Run + await executeTestsForProject( + project, + [classItem], + runMock.object, + request, + deps, + expandExcludeSet([leaf2]), + ); + + // Assert - only leaf1 is started and executed + runMock.verify((r) => r.started(leaf1), typemoq.Times.once()); + runMock.verify((r) => r.started(leaf2), typemoq.Times.never()); + const passedTestIds = project.executionAdapterStub.firstCall.args[1] as string[]; + expect(passedTestIds).to.deep.equal(['runId1']); + }); + + test('should not run anything when the whole selection is excluded', async () => { + // Mock - the requested class is itself excluded + const project = createMockProjectAdapter({ projectPath: '/workspace/proj', projectName: 'proj' }); + const leaf1 = createMockTestItem('test1', '/workspace/proj/test.py'); + const leaf2 = createMockTestItem('test2', '/workspace/proj/test.py'); + const classItem = createMockTestItem('TestClass', '/workspace/proj/test.py', [leaf1, leaf2]); + project.resultResolver.vsIdToRunId.set('test1', 'runId1'); + project.resultResolver.vsIdToRunId.set('test2', 'runId2'); + const runMock = createMockTestRun(); + const request = { profile: { kind: TestRunProfileKind.Run } } as TestRunRequest; + const deps = createMockDependencies(); + + // Run + await executeTestsForProject( + project, + [classItem], + runMock.object, + request, + deps, + expandExcludeSet([classItem]), + ); + + // Assert - nothing is started and the adapter is never called (an empty ID list would run all tests) + runMock.verify((r) => r.started(typemoq.It.isAny()), typemoq.Times.never()); + expect(project.executionAdapterStub.called).to.be.false; + }); + + test('should still deduplicate test IDs when exclusions are used', async () => { + // Mock - the class and one of its methods are both selected, the other method is excluded + const project = createMockProjectAdapter({ projectPath: '/workspace/proj', projectName: 'proj' }); + const leaf1 = createMockTestItem('test1', '/workspace/proj/test.py'); + const leaf2 = createMockTestItem('test2', '/workspace/proj/test.py'); + const classItem = createMockTestItem('TestClass', '/workspace/proj/test.py', [leaf1, leaf2]); + project.resultResolver.vsIdToRunId.set('test1', 'runId1'); + project.resultResolver.vsIdToRunId.set('test2', 'runId2'); + const runMock = createMockTestRun(); + const request = { profile: { kind: TestRunProfileKind.Run } } as TestRunRequest; + const deps = createMockDependencies(); + + // Run + await executeTestsForProject( + project, + [classItem, leaf1], + runMock.object, + request, + deps, + expandExcludeSet([leaf2]), + ); + + // Assert - leaf1 is started and sent once + runMock.verify((r) => r.started(leaf1), typemoq.Times.once()); + const passedTestIds = project.executionAdapterStub.firstCall.args[1] as string[]; + expect(passedTestIds).to.deep.equal(['runId1']); + }); }); // ===== executeTestsForProjects Tests ===== @@ -623,6 +723,58 @@ suite('Project Test Execution', () => { const telemetryProps = runDoneCalls[0].args[2]; expect(telemetryProps.debugging).to.be.true; }); + + test('should respect request.exclude when executing tests', async () => { + // Mock - project with two test items, one excluded via request.exclude + const project = createMockProjectAdapter({ projectPath: '/workspace/proj', projectName: 'proj' }); + const item1 = createMockTestItem('test1', '/workspace/proj/test.py'); + const item2 = createMockTestItem('test2', '/workspace/proj/test.py'); + project.resultResolver.vsIdToRunId.set('test1', 'runId1'); + project.resultResolver.vsIdToRunId.set('test2', 'runId2'); + const runMock = createMockTestRun(); + const token = new CancellationTokenSource().token; + const request = ({ + profile: { kind: TestRunProfileKind.Run }, + exclude: [item2], + } as unknown) as TestRunRequest; + const deps = createMockDependencies(); + + // Run + await executeTestsForProjects([project], [item1, item2], runMock.object, request, token, deps); + + // Assert - only item1 is executed + runMock.verify((r) => r.started(item1), typemoq.Times.once()); + runMock.verify((r) => r.started(item2), typemoq.Times.never()); + const passedTestIds = project.executionAdapterStub.firstCall.args[1] as string[]; + expect(passedTestIds).to.deep.equal(['runId1']); + }); + + test('should only skip the project whose tests are all excluded', async () => { + // Mock - two projects with one test each, the test in proj1 is excluded + const proj1 = createMockProjectAdapter({ projectPath: '/workspace/proj1', projectName: 'proj1' }); + const proj2 = createMockProjectAdapter({ projectPath: '/workspace/proj2', projectName: 'proj2' }); + const item1 = createMockTestItem('test1', '/workspace/proj1/test.py'); + const item2 = createMockTestItem('test2', '/workspace/proj2/test.py'); + proj1.resultResolver.vsIdToRunId.set('test1', 'runId1'); + proj2.resultResolver.vsIdToRunId.set('test2', 'runId2'); + const runMock = createMockTestRun(); + const token = new CancellationTokenSource().token; + const request = ({ + profile: { kind: TestRunProfileKind.Run }, + exclude: [item1], + } as unknown) as TestRunRequest; + const deps = createMockDependencies(); + + // Run + await executeTestsForProjects([proj1, proj2], [item1, item2], runMock.object, request, token, deps); + + // Assert - proj1 is not run at all, proj2 runs item2 + runMock.verify((r) => r.started(item1), typemoq.Times.never()); + runMock.verify((r) => r.started(item2), typemoq.Times.once()); + expect(proj1.executionAdapterStub.called).to.be.false; + expect(proj2.executionAdapterStub.calledOnce).to.be.true; + expect(proj2.executionAdapterStub.firstCall.args[1]).to.deep.equal(['runId2']); + }); }); // ===== setupCoverageForProjects Tests ===== diff --git a/src/test/testing/testController/testItemUtilities.unit.test.ts b/src/test/testing/testController/testItemUtilities.unit.test.ts new file mode 100644 index 000000000000..616b2a68970b --- /dev/null +++ b/src/test/testing/testController/testItemUtilities.unit.test.ts @@ -0,0 +1,82 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { TestItem } from 'vscode'; +import { + DebugTestTag, + expandExcludeSet, + getTestCaseNodes, + RunTestTag, +} from '../../../client/testing/testController/common/testItemUtilities'; + +function createMockTestItem(id: string, canResolveChildren: boolean, children: TestItem[] = []): TestItem { + return ({ + id, + canResolveChildren, + tags: [RunTestTag, DebugTestTag], + children: { + forEach: (callback: (item: TestItem) => void) => children.forEach(callback), + size: children.length, + }, + } as unknown) as TestItem; +} + +suite('expandExcludeSet', () => { + test('should return undefined when there is nothing to exclude', () => { + assert.strictEqual(expandExcludeSet(undefined), undefined); + assert.strictEqual(expandExcludeSet([]), undefined); + }); + + test('should include the excluded item itself', () => { + const item = createMockTestItem('leaf', false); + const result = expandExcludeSet([item]); + assert.deepStrictEqual(result, new Set([item])); + }); + + test('should include all descendants of excluded items', () => { + const grandchild = createMockTestItem('grandchild', false); + const child1 = createMockTestItem('child1', true, [grandchild]); + const child2 = createMockTestItem('child2', false); + const parent = createMockTestItem('parent', true, [child1, child2]); + const result = expandExcludeSet([parent]); + assert.deepStrictEqual(result, new Set([parent, child1, child2, grandchild])); + }); + + test('should handle overlapping excluded items', () => { + const child = createMockTestItem('child', false); + const parent = createMockTestItem('parent', true, [child]); + const result = expandExcludeSet([parent, child]); + assert.deepStrictEqual(result, new Set([parent, child])); + }); +}); + +suite('getTestCaseNodes with an exclude set', () => { + test('should collect leaf test nodes without an exclude set', () => { + const leaf1 = createMockTestItem('leaf1', false); + const leaf2 = createMockTestItem('leaf2', false); + const parent = createMockTestItem('parent', true, [leaf1, leaf2]); + assert.deepStrictEqual(getTestCaseNodes(parent), [leaf1, leaf2]); + }); + + test('should skip excluded leaf nodes', () => { + const leaf1 = createMockTestItem('leaf1', false); + const leaf2 = createMockTestItem('leaf2', false); + const parent = createMockTestItem('parent', true, [leaf1, leaf2]); + assert.deepStrictEqual(getTestCaseNodes(parent, [], expandExcludeSet([leaf1])), [leaf2]); + }); + + test('should skip the whole subtree of an excluded node', () => { + const leaf1 = createMockTestItem('leaf1', false); + const leaf2 = createMockTestItem('leaf2', false); + const folder = createMockTestItem('folder', true, [leaf1]); + const root = createMockTestItem('root', true, [folder, leaf2]); + assert.deepStrictEqual(getTestCaseNodes(root, [], expandExcludeSet([folder])), [leaf2]); + }); + + test('should return nothing when the requested node is excluded', () => { + const leaf = createMockTestItem('leaf', false); + const root = createMockTestItem('root', true, [leaf]); + assert.deepStrictEqual(getTestCaseNodes(root, [], expandExcludeSet([root])), []); + }); +}); diff --git a/src/test/testing/testController/workspaceTestAdapter.unit.test.ts b/src/test/testing/testController/workspaceTestAdapter.unit.test.ts index 0f5c0c2a2c4c..bce6dc957cbe 100644 --- a/src/test/testing/testController/workspaceTestAdapter.unit.test.ts +++ b/src/test/testing/testController/workspaceTestAdapter.unit.test.ts @@ -357,7 +357,7 @@ suite('Workspace test adapter', () => { ); resultResolver.runIdToVSid.set('mockTestItem1', 'mockTestItem1'); - sinon.stub(testItemUtilities, 'getTestCaseNodes').callsFake((testNode: TestItem) => + sandbox.stub(testItemUtilities, 'getTestCaseNodes').callsFake((testNode: TestItem) => // Custom implementation logic here based on the provided testNode and collection // Example implementation: returning a predefined array of TestItem objects @@ -414,6 +414,66 @@ suite('Workspace test adapter', () => { sinon.assert.calledOnce(executionTestsStub); }); + test('Excluded tests are not started or sent to the execution adapter', async () => { + const workspaceTestAdapter = new WorkspaceTestAdapter( + 'pytest', + new UnittestTestDiscoveryAdapter(stubConfigSettings), + new UnittestTestExecutionAdapter(stubConfigSettings), + Uri.parse('foo'), + resultResolver, + ); + const test1 = createRunnableTestItem('test1'); + const test2 = createRunnableTestItem('test2'); + const testClass = createRunnableTestItem('TestClass', [test1, test2]); + resultResolver.vsIdToRunId.set('test1', 'run1'); + resultResolver.vsIdToRunId.set('test2', 'run2'); + + await workspaceTestAdapter.executeTests( + testController, + runInstance.object, + [testClass], + execFactory.object, + undefined, + undefined, + undefined, + undefined, + [test2], + ); + + runInstance.verify((r) => r.started(test1), typemoq.Times.once()); + runInstance.verify((r) => r.started(test2), typemoq.Times.never()); + sinon.assert.calledOnce(executionTestsStub); + assert.deepStrictEqual(executionTestsStub.firstCall.args[1], ['run1']); + }); + + test('If every requested test is excluded, the execution adapter is not called', async () => { + const workspaceTestAdapter = new WorkspaceTestAdapter( + 'pytest', + new UnittestTestDiscoveryAdapter(stubConfigSettings), + new UnittestTestExecutionAdapter(stubConfigSettings), + Uri.parse('foo'), + resultResolver, + ); + const test1 = createRunnableTestItem('test1'); + const testClass = createRunnableTestItem('TestClass', [test1]); + resultResolver.vsIdToRunId.set('test1', 'run1'); + + await workspaceTestAdapter.executeTests( + testController, + runInstance.object, + [testClass], + execFactory.object, + undefined, + undefined, + undefined, + undefined, + [testClass], + ); + + runInstance.verify((r) => r.started(typemoq.It.isAny()), typemoq.Times.never()); + sinon.assert.notCalled(executionTestsStub); + }); + test('If execution is already running, do not call executionAdapter.runTests again', async () => { executionTestsStub.callsFake( async () => @@ -521,3 +581,17 @@ function createMockTestItem(id: string): TestItem { return mockTestItem; } + +/** A test item that getTestCaseNodes treats as runnable: a leaf with tags, or a parent that can resolve children. */ +function createRunnableTestItem(id: string, children: TestItem[] = []): TestItem { + return ({ + id, + canResolveChildren: children.length > 0, + tags: [testItemUtilities.RunTestTag], + children: { + size: children.length, + forEach: (callback: (item: TestItem) => void) => children.forEach(callback), + }, + uri: Uri.file('/foo/bar'), + } as unknown) as TestItem; +}