diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index 82856627e0c9..b92e7a870f20 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -99,6 +99,11 @@ export class PythonResultResolver implements ITestResultResolver { // if any tests exist, they should be populated in the test tree, regardless of whether there were errors or not. // parse and insert test data. + // Clear existing mappings before rebuilding test tree + this.runIdToTestItem.clear(); + this.runIdToVSid.clear(); + this.vsIdToRunId.clear(); + // If the test root for this folder exists: Workspace refresh, update its children. // Otherwise, it is a freshly discovered workspace, and we need to create a new test root and populate the test tree. populateTestTree(this.testController, rawTestData.tests, undefined, this, token); @@ -173,165 +178,291 @@ export class PythonResultResolver implements ITestResultResolver { } } + /** + * Collect all test case items from the test controller tree. + * Note: This performs full tree traversal - use cached lookups when possible. + */ + private collectAllTestCases(): TestItem[] { + const testCases: TestItem[] = []; + + this.testController.items.forEach((i) => { + const tempArr: TestItem[] = getTestCaseNodes(i); + testCases.push(...tempArr); + }); + + return testCases; + } + + /** + * Find a test item efficiently using cached maps with fallback strategies. + * Uses a three-tier approach: direct lookup, ID mapping, then tree search. + */ + private findTestItemByIdEfficient(keyTemp: string): TestItem | undefined { + // Try direct O(1) lookup first + const directItem = this.runIdToTestItem.get(keyTemp); + if (directItem) { + // Validate the item is still in the test tree + if (this.isTestItemValid(directItem)) { + return directItem; + } else { + // Clean up stale reference + this.runIdToTestItem.delete(keyTemp); + } + } + + // Try vsId mapping as fallback + const vsId = this.runIdToVSid.get(keyTemp); + if (vsId) { + // Search by VS Code ID in the controller + let foundItem: TestItem | undefined; + this.testController.items.forEach((item) => { + if (item.id === vsId) { + foundItem = item; + return; + } + if (!foundItem) { + item.children.forEach((child) => { + if (child.id === vsId) { + foundItem = child; + } + }); + } + }); + + if (foundItem) { + // Cache for future lookups + this.runIdToTestItem.set(keyTemp, foundItem); + return foundItem; + } else { + // Clean up stale mapping + this.runIdToVSid.delete(keyTemp); + this.vsIdToRunId.delete(vsId); + } + } + + // Last resort: full tree search + traceError(`Falling back to tree search for test: ${keyTemp}`); + const testCases = this.collectAllTestCases(); + return testCases.find((item) => item.id === vsId); + } + + /** + * Check if a TestItem is still valid (exists in the TestController tree) + * + * Time Complexity: O(depth) where depth is the maximum nesting level of the test tree. + * In most cases this is O(1) to O(3) since test trees are typically shallow. + */ + private isTestItemValid(testItem: TestItem): boolean { + // Simple validation: check if the item's parent chain leads back to the controller + let current: TestItem | undefined = testItem; + while (current?.parent) { + current = current.parent; + } + + // If we reached a root item, check if it's in the controller + if (current) { + return this.testController.items.get(current.id) === current; + } + + // If no parent chain, check if it's directly in the controller + return this.testController.items.get(testItem.id) === testItem; + } + + /** + * Clean up stale test item references from the cache maps. + * Validates cached items and removes any that are no longer in the test tree. + */ + public cleanupStaleReferences(): void { + const staleRunIds: string[] = []; + + // Check all runId->TestItem mappings + this.runIdToTestItem.forEach((testItem, runId) => { + if (!this.isTestItemValid(testItem)) { + staleRunIds.push(runId); + } + }); + + // Remove stale entries + staleRunIds.forEach((runId) => { + const vsId = this.runIdToVSid.get(runId); + this.runIdToTestItem.delete(runId); + this.runIdToVSid.delete(runId); + if (vsId) { + this.vsIdToRunId.delete(vsId); + } + }); + + if (staleRunIds.length > 0) { + traceVerbose(`Cleaned up ${staleRunIds.length} stale test item references`); + } + } + + /** + * Handle test items that errored during execution. + * Extracts error details, finds the corresponding TestItem, and reports the error to VS Code's Test Explorer. + */ + private handleTestError(keyTemp: string, testItem: any, runInstance: TestRun): void { + const rawTraceback = testItem.traceback ?? ''; + const traceback = splitLines(rawTraceback, { + trim: false, + removeEmptyEntries: true, + }).join('\r\n'); + const text = `${testItem.test} failed with error: ${testItem.message ?? testItem.outcome}\r\n${traceback}`; + const message = new TestMessage(text); + + const foundItem = this.findTestItemByIdEfficient(keyTemp); + + if (foundItem?.uri) { + if (foundItem.range) { + message.location = new Location(foundItem.uri, foundItem.range); + } + runInstance.errored(foundItem, message); + } + } + + /** + * Handle test items that failed during execution + */ + private handleTestFailure(keyTemp: string, testItem: any, runInstance: TestRun): void { + const rawTraceback = testItem.traceback ?? ''; + const traceback = splitLines(rawTraceback, { + trim: false, + removeEmptyEntries: true, + }).join('\r\n'); + + const text = `${testItem.test} failed: ${testItem.message ?? testItem.outcome}\r\n${traceback}`; + const message = new TestMessage(text); + + const foundItem = this.findTestItemByIdEfficient(keyTemp); + + if (foundItem?.uri) { + if (foundItem.range) { + message.location = new Location(foundItem.uri, foundItem.range); + } + runInstance.failed(foundItem, message); + } + } + + /** + * Handle test items that passed during execution + */ + private handleTestSuccess(keyTemp: string, runInstance: TestRun): void { + const grabTestItem = this.runIdToTestItem.get(keyTemp); + + if (grabTestItem !== undefined) { + const foundItem = this.findTestItemByIdEfficient(keyTemp); + if (foundItem?.uri) { + runInstance.passed(grabTestItem); + } + } + } + + /** + * Handle test items that were skipped during execution + */ + private handleTestSkipped(keyTemp: string, runInstance: TestRun): void { + const grabTestItem = this.runIdToTestItem.get(keyTemp); + + if (grabTestItem !== undefined) { + const foundItem = this.findTestItemByIdEfficient(keyTemp); + if (foundItem?.uri) { + runInstance.skipped(grabTestItem); + } + } + } + + /** + * Handle subtest failures + */ + private handleSubtestFailure(keyTemp: string, testItem: any, runInstance: TestRun): void { + const [parentTestCaseId, subtestId] = splitTestNameWithRegex(keyTemp); + const parentTestItem = this.runIdToTestItem.get(parentTestCaseId); + + if (parentTestItem) { + const subtestStats = this.subTestStats.get(parentTestCaseId); + if (subtestStats) { + subtestStats.failed += 1; + } else { + this.subTestStats.set(parentTestCaseId, { + failed: 1, + passed: 0, + }); + clearAllChildren(parentTestItem); + } + + const subTestItem = this.testController?.createTestItem(subtestId, subtestId, parentTestItem.uri); + + if (subTestItem) { + const traceback = testItem.traceback ?? ''; + const text = `${testItem.subtest} failed: ${testItem.message ?? testItem.outcome}\r\n${traceback}`; + parentTestItem.children.add(subTestItem); + runInstance.started(subTestItem); + const message = new TestMessage(text); + if (parentTestItem.uri && parentTestItem.range) { + message.location = new Location(parentTestItem.uri, parentTestItem.range); + } + runInstance.failed(subTestItem, message); + } else { + throw new Error('Unable to create new child node for subtest'); + } + } else { + throw new Error('Parent test item not found'); + } + } + + /** + * Handle subtest successes + */ + private handleSubtestSuccess(keyTemp: string, runInstance: TestRun): void { + const [parentTestCaseId, subtestId] = splitTestNameWithRegex(keyTemp); + const parentTestItem = this.runIdToTestItem.get(parentTestCaseId); + + if (parentTestItem) { + const subtestStats = this.subTestStats.get(parentTestCaseId); + if (subtestStats) { + subtestStats.passed += 1; + } else { + this.subTestStats.set(parentTestCaseId, { failed: 0, passed: 1 }); + clearAllChildren(parentTestItem); + } + + const subTestItem = this.testController?.createTestItem(subtestId, subtestId, parentTestItem.uri); + + if (subTestItem) { + parentTestItem.children.add(subTestItem); + runInstance.started(subTestItem); + runInstance.passed(subTestItem); + } else { + throw new Error('Unable to create new child node for subtest'); + } + } else { + throw new Error('Parent test item not found'); + } + } + + /** + * Process test execution results and update VS Code's Test Explorer with outcomes. + * Uses efficient lookup methods to handle large numbers of test results. + */ public _resolveExecution(payload: ExecutionTestPayload, runInstance: TestRun): void { const rawTestExecData = payload as ExecutionTestPayload; if (rawTestExecData !== undefined && rawTestExecData.result !== undefined) { - // Map which holds the subtest information for each test item. - - // iterate through payload and update the UI accordingly. for (const keyTemp of Object.keys(rawTestExecData.result)) { - const testCases: TestItem[] = []; - - // grab leaf level test items - this.testController.items.forEach((i) => { - const tempArr: TestItem[] = getTestCaseNodes(i); - testCases.push(...tempArr); - }); const testItem = rawTestExecData.result[keyTemp]; + // Delegate to specific outcome handlers using efficient lookups if (testItem.outcome === 'error') { - const rawTraceback = testItem.traceback ?? ''; - const traceback = splitLines(rawTraceback, { - trim: false, - removeEmptyEntries: true, - }).join('\r\n'); - const text = `${testItem.test} failed with error: ${ - testItem.message ?? testItem.outcome - }\r\n${traceback}`; - const message = new TestMessage(text); - - const grabVSid = this.runIdToVSid.get(keyTemp); - // search through freshly built array of testItem to find the failed test and update UI. - testCases.forEach((indiItem) => { - if (indiItem.id === grabVSid) { - if (indiItem.uri) { - if (indiItem.range) { - message.location = new Location(indiItem.uri, indiItem.range); - } - runInstance.errored(indiItem, message); - } - } - }); + this.handleTestError(keyTemp, testItem, runInstance); } else if (testItem.outcome === 'failure' || testItem.outcome === 'passed-unexpected') { - const rawTraceback = testItem.traceback ?? ''; - const traceback = splitLines(rawTraceback, { - trim: false, - removeEmptyEntries: true, - }).join('\r\n'); - - const text = `${testItem.test} failed: ${testItem.message ?? testItem.outcome}\r\n${traceback}`; - const message = new TestMessage(text); - - // note that keyTemp is a runId for unittest library... - const grabVSid = this.runIdToVSid.get(keyTemp); - // search through freshly built array of testItem to find the failed test and update UI. - testCases.forEach((indiItem) => { - if (indiItem.id === grabVSid) { - if (indiItem.uri) { - if (indiItem.range) { - message.location = new Location(indiItem.uri, indiItem.range); - } - runInstance.failed(indiItem, message); - } - } - }); + this.handleTestFailure(keyTemp, testItem, runInstance); } else if (testItem.outcome === 'success' || testItem.outcome === 'expected-failure') { - const grabTestItem = this.runIdToTestItem.get(keyTemp); - const grabVSid = this.runIdToVSid.get(keyTemp); - if (grabTestItem !== undefined) { - testCases.forEach((indiItem) => { - if (indiItem.id === grabVSid) { - if (indiItem.uri) { - runInstance.passed(grabTestItem); - } - } - }); - } + this.handleTestSuccess(keyTemp, runInstance); } else if (testItem.outcome === 'skipped') { - const grabTestItem = this.runIdToTestItem.get(keyTemp); - const grabVSid = this.runIdToVSid.get(keyTemp); - if (grabTestItem !== undefined) { - testCases.forEach((indiItem) => { - if (indiItem.id === grabVSid) { - if (indiItem.uri) { - runInstance.skipped(grabTestItem); - } - } - }); - } + this.handleTestSkipped(keyTemp, runInstance); } else if (testItem.outcome === 'subtest-failure') { - // split on [] or () based on how the subtest is setup. - const [parentTestCaseId, subtestId] = splitTestNameWithRegex(keyTemp); - const parentTestItem = this.runIdToTestItem.get(parentTestCaseId); - const data = testItem; - // find the subtest's parent test item - if (parentTestItem) { - const subtestStats = this.subTestStats.get(parentTestCaseId); - if (subtestStats) { - subtestStats.failed += 1; - } else { - this.subTestStats.set(parentTestCaseId, { - failed: 1, - passed: 0, - }); - // clear since subtest items don't persist between runs - clearAllChildren(parentTestItem); - } - const subTestItem = this.testController?.createTestItem( - subtestId, - subtestId, - parentTestItem.uri, - ); - // create a new test item for the subtest - if (subTestItem) { - const traceback = data.traceback ?? ''; - const text = `${data.subtest} failed: ${ - testItem.message ?? testItem.outcome - }\r\n${traceback}`; - parentTestItem.children.add(subTestItem); - runInstance.started(subTestItem); - const message = new TestMessage(text); - if (parentTestItem.uri && parentTestItem.range) { - message.location = new Location(parentTestItem.uri, parentTestItem.range); - } - runInstance.failed(subTestItem, message); - } else { - throw new Error('Unable to create new child node for subtest'); - } - } else { - throw new Error('Parent test item not found'); - } + this.handleSubtestFailure(keyTemp, testItem, runInstance); } else if (testItem.outcome === 'subtest-success') { - // split on [] or () based on how the subtest is setup. - const [parentTestCaseId, subtestId] = splitTestNameWithRegex(keyTemp); - const parentTestItem = this.runIdToTestItem.get(parentTestCaseId); - - // find the subtest's parent test item - if (parentTestItem) { - const subtestStats = this.subTestStats.get(parentTestCaseId); - if (subtestStats) { - subtestStats.passed += 1; - } else { - this.subTestStats.set(parentTestCaseId, { failed: 0, passed: 1 }); - // clear since subtest items don't persist between runs - clearAllChildren(parentTestItem); - } - const subTestItem = this.testController?.createTestItem( - subtestId, - subtestId, - parentTestItem.uri, - ); - // create a new test item for the subtest - if (subTestItem) { - parentTestItem.children.add(subTestItem); - runInstance.started(subTestItem); - runInstance.passed(subTestItem); - } else { - throw new Error('Unable to create new child node for subtest'); - } - } else { - throw new Error('Parent test item not found'); - } + this.handleSubtestSuccess(keyTemp, runInstance); } } } diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index dcd78dc23dba..97c04d5dfdf1 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -9,7 +9,11 @@ import * as fs from 'fs'; import * as os from 'os'; import * as sinon from 'sinon'; import { PytestTestDiscoveryAdapter } from '../../../client/testing/testController/pytest/pytestDiscoveryAdapter'; -import { ITestController, ITestResultResolver } from '../../../client/testing/testController/common/types'; +import { + ITestController, + ITestResultResolver, + ExecutionTestPayload, +} from '../../../client/testing/testController/common/types'; import { IPythonExecutionFactory } from '../../../client/common/process/types'; import { IConfigurationService } from '../../../client/common/types'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -1033,4 +1037,206 @@ suite('End to End Tests: test adapters', () => { assert.strictEqual(failureOccurred, false, failureMsg); }); }); + + test('_resolveExecution performance test: validates efficient test result processing', async () => { + // This test validates that _resolveExecution processes test results efficiently + // without expensive tree rebuilding or linear searching operations. + // + // The test ensures that processing many test results (like parameterized tests) + // remains fast and doesn't cause performance issues or stack overflow. + + // ================================================================ + // SETUP: Initialize test environment and tracking variables + // ================================================================ + resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); + + // Performance tracking variables + let totalCallTime = 0; + let callCount = 0; + const callTimes: number[] = []; + let treeRebuildCount = 0; + let totalSearchOperations = 0; + + // Test configuration - Moderate scale to validate efficiency + const numTestFiles = 5; // Multiple test files + const testFunctionsPerFile = 10; // Test functions per file + const totalTestItems = numTestFiles * testFunctionsPerFile; // Total test items in mock tree + const numParameterizedResults = 15; // Number of parameterized test results to process + + // ================================================================ + // MOCK: Set up spies and function wrapping to track performance + // ================================================================ + + // Mock getTestCaseNodes to track expensive tree operations + const originalGetTestCaseNodes = require('../../../client/testing/testController/common/testItemUtilities') + .getTestCaseNodes; + const getTestCaseNodesSpy = sinon.stub().callsFake((item) => { + treeRebuildCount++; + const result = originalGetTestCaseNodes(item); + // Track search operations through tree items + // Safely handle undefined results + if (result && Array.isArray(result)) { + totalSearchOperations += result.length; + } + return result || []; // Return empty array if undefined + }); + + // Replace the real function with our spy + const testItemUtilities = require('../../../client/testing/testController/common/testItemUtilities'); + testItemUtilities.getTestCaseNodes = getTestCaseNodesSpy; + + // Wrap the _resolveExecution function to measure performance + const original_resolveExecution = resultResolver._resolveExecution.bind(resultResolver); + resultResolver._resolveExecution = async (payload, runInstance) => { + const startTime = performance.now(); + callCount++; + + // Call the actual implementation + await original_resolveExecution(payload, runInstance); + + const endTime = performance.now(); + const callTime = endTime - startTime; + callTimes.push(callTime); + totalCallTime += callTime; + + return Promise.resolve(); + }; + + // ================================================================ + // SETUP: Create test data that simulates realistic test scenarios + // ================================================================ + + // Create a mock TestController with the methods we need + const mockTestController = { + items: new Map(), + createTestItem: (id: string, label: string, uri?: Uri) => { + const childrenMap = new Map(); + // Add forEach method to children map to simulate TestItemCollection + (childrenMap as any).forEach = function (callback: (item: any) => void) { + Map.prototype.forEach.call(this, callback); + }; + + const mockTestItem = { + id, + label, + uri, + children: childrenMap, + parent: undefined, + canResolveChildren: false, + tags: [{ id: 'python-run' }, { id: 'python-debug' }], + }; + return mockTestItem; + }, + // Add a forEach method to simulate the problematic iteration + forEach: function (callback: (item: any) => void) { + this.items.forEach(callback); + }, + }; // Replace the testController in our resolver + (resultResolver as any).testController = mockTestController; + + // Create test controller with many test items (simulates real workspace) + for (let i = 0; i < numTestFiles; i++) { + const testItem = mockTestController.createTestItem( + `test_file_${i}`, + `Test File ${i}`, + Uri.file(`/test_${i}.py`), + ); + mockTestController.items.set(`test_file_${i}`, testItem); + + // Add child test items to each file + for (let j = 0; j < testFunctionsPerFile; j++) { + const childItem = mockTestController.createTestItem( + `test_${i}_${j}`, + `test_method_${j}`, + Uri.file(`/test_${i}.py`), + ); + testItem.children.set(`test_${i}_${j}`, childItem); + + // Set up the ID mappings that the resolver uses + resultResolver.runIdToTestItem.set(`test_${i}_${j}`, childItem as any); + resultResolver.runIdToVSid.set(`test_${i}_${j}`, `test_${i}_${j}`); + resultResolver.vsIdToRunId.set(`test_${i}_${j}`, `test_${i}_${j}`); + } + } // Create payload with multiple test results (simulates real test execution) + const testResults: Record = {}; + for (let i = 0; i < numParameterizedResults; i++) { + testResults[`test_0_${i % 20}`] = { + test: `test_method[${i}]`, + outcome: 'success', + message: null, + traceback: null, + subtest: null, + }; + } + + const payload: ExecutionTestPayload = { + cwd: '/test', + status: 'success' as const, + error: '', + result: testResults, + }; + + const mockRunInstance = { + passed: sinon.stub(), + failed: sinon.stub(), + errored: sinon.stub(), + skipped: sinon.stub(), + }; + + // ================================================================ + // EXECUTION: Run the performance test + // ================================================================ + + const overallStartTime = performance.now(); + + // Run the _resolveExecution function with test data + await resultResolver._resolveExecution(payload, mockRunInstance as any); + + const overallEndTime = performance.now(); + const totalTime = overallEndTime - overallStartTime; + + // ================================================================ + // CLEANUP: Restore original functions + // ================================================================ + testItemUtilities.getTestCaseNodes = originalGetTestCaseNodes; + + // ================================================================ + // ASSERT: Verify efficient performance characteristics + // ================================================================ + console.log(`\n=== PERFORMANCE RESULTS ===`); + console.log( + `Test setup: ${numTestFiles} files × ${testFunctionsPerFile} test functions = ${totalTestItems} total items`, + ); + console.log(`Total execution time: ${totalTime.toFixed(2)}ms`); + console.log(`Tree operations performed: ${treeRebuildCount}`); + console.log(`Search operations: ${totalSearchOperations}`); + console.log(`Average time per call: ${(totalCallTime / callCount).toFixed(2)}ms`); + console.log(`Results processed: ${numParameterizedResults}`); + + // Basic function call verification + assert.strictEqual(callCount, 1, 'Expected _resolveExecution to be called once'); + + // EFFICIENCY VERIFICATION: Ensure minimal expensive operations + assert.strictEqual( + treeRebuildCount, + 0, + 'Expected ZERO tree rebuilds - efficient implementation should use cached lookups', + ); + + assert.strictEqual( + totalSearchOperations, + 0, + 'Expected ZERO linear search operations - efficient implementation should use direct lookups', + ); + + // Performance threshold verification - should be fast + assert.ok(totalTime < 100, `Function should complete quickly, took ${totalTime}ms (should be under 100ms)`); + + // Scalability check - time should not grow significantly with more results + const timePerResult = totalTime / numParameterizedResults; + assert.ok( + timePerResult < 10, + `Time per result should be minimal: ${timePerResult.toFixed(2)}ms per result (should be under 10ms)`, + ); + }); });