From 55fd837625ea8f00a0cab62def356d36f39653af Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Thu, 18 Sep 2025 15:07:10 -0700 Subject: [PATCH 1/5] Optimize test result processing in PythonResultResolver to improve performance and reduce complexity --- .../testController/common/resultResolver.ts | 444 ++++++++++++------ .../testing/common/testingAdapter.test.ts | 208 +++++++- 2 files changed, 506 insertions(+), 146 deletions(-) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index 82856627e0c9..3e63cf0d46e4 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. + // CLEANUP: Clear existing maps to remove stale references before rebuilding + 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,314 @@ export class PythonResultResolver implements ITestResultResolver { } } + /** + * PERFORMANCE CRITICAL: This method rebuilds the entire test case array + * Currently called for EVERY test result, causing O(n*m*k) complexity + * TODO: Replace with cached lookup or direct item access + */ + private collectAllTestCases(): TestItem[] { + const testCases: TestItem[] = []; + + // PERFORMANCE PROBLEM: This rebuilds the ENTIRE test case array + // MIDDLE OPERATION: O(m) where m = number of top-level test items in controller + this.testController.items.forEach((i) => { + // RECURSIVE TREE TRAVERSAL: getTestCaseNodes(i) is O(depth * children) + // For parameterized tests with subtests, this can be very deep + const tempArr: TestItem[] = getTestCaseNodes(i); + testCases.push(...tempArr); + }); + + return testCases; + } + + /** + * Find a test item efficiently using the pre-built maps, with fallback to tree search only if needed. + * This avoids the O(k) search for 99% of cases while still handling edge cases. + */ + private findTestItemByIdEfficient(keyTemp: string): TestItem | undefined { + // FAST PATH: Try the O(1) lookup first + const directItem = this.runIdToTestItem.get(keyTemp); + if (directItem) { + // VALIDATION: Check if the TestItem is still valid (hasn't been deleted from controller) + // This prevents using stale references + if (this.isTestItemValid(directItem)) { + return directItem; + } else { + // Clean up stale reference + this.runIdToTestItem.delete(keyTemp); + } + } + + // FALLBACK: Try vsId mapping + const vsId = this.runIdToVSid.get(keyTemp); + if (vsId) { + // Try to find by VS Code ID directly in the controller + // This is still much faster than full tree traversal + let foundItem: TestItem | undefined; + this.testController.items.forEach((item) => { + if (item.id === vsId) { + foundItem = item; + return; + } + // Check children only if not found at top level + if (!foundItem) { + item.children.forEach((child) => { + if (child.id === vsId) { + foundItem = child; + } + }); + } + }); + + if (foundItem) { + // Cache for next time to avoid this lookup + this.runIdToTestItem.set(keyTemp, foundItem); + return foundItem; + } else { + // Clean up stale vsId mapping + this.runIdToVSid.delete(keyTemp); + this.vsIdToRunId.delete(vsId); + } + } + + // LAST RESORT: Only do expensive tree traversal if really needed + // This should rarely happen with proper discovery + console.warn(`Falling back to expensive 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 references from maps (optional method for external cleanup) + * + * Time Complexity: O(n * depth) where n is the number of cached test items and depth + * is the average tree depth. This is much more efficient than the original O(n*m*k) + * tree rebuilding approach, since it only validates existing cache entries. + */ + 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) { + console.log(`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. + * + * CURRENT PERFORMANCE ISSUE: For each test result, this method rebuilds the entire test tree + * (O(m) traversal) and then searches through all test items (O(k) search). With parameterized + * tests producing many results, this becomes O(n*m*k) complexity, eventually causing stack overflow. + */ public _resolveExecution(payload: ExecutionTestPayload, runInstance: TestRun): void { + // PERFORMANCE ISSUE: This method has O(n*m*k) complexity that causes stack overflow: + // - For each test result (n), we rebuild the entire test tree (m items) + // - Then search through all leaf nodes (k nodes) to find the matching test + // - With parameterized tests, n can be large, making this exponentially slow 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. + // PERFORMANCE FIX: No longer need to rebuild test tree for every result! + // Use efficient lookup methods instead 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)`, + ); + }); }); From 26e5a3b49b63a230ff5bd5ba672d372ed61e8143 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Thu, 18 Sep 2025 15:11:16 -0700 Subject: [PATCH 2/5] update comments --- .../testController/common/resultResolver.ts | 55 ++++++------------- 1 file changed, 16 insertions(+), 39 deletions(-) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index 3e63cf0d46e4..e7c42eb0d69d 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -99,7 +99,7 @@ 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. - // CLEANUP: Clear existing maps to remove stale references before rebuilding + // Clear existing mappings before rebuilding test tree this.runIdToTestItem.clear(); this.runIdToVSid.clear(); this.vsIdToRunId.clear(); @@ -179,18 +179,13 @@ export class PythonResultResolver implements ITestResultResolver { } /** - * PERFORMANCE CRITICAL: This method rebuilds the entire test case array - * Currently called for EVERY test result, causing O(n*m*k) complexity - * TODO: Replace with cached lookup or direct item access + * 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[] = []; - // PERFORMANCE PROBLEM: This rebuilds the ENTIRE test case array - // MIDDLE OPERATION: O(m) where m = number of top-level test items in controller this.testController.items.forEach((i) => { - // RECURSIVE TREE TRAVERSAL: getTestCaseNodes(i) is O(depth * children) - // For parameterized tests with subtests, this can be very deep const tempArr: TestItem[] = getTestCaseNodes(i); testCases.push(...tempArr); }); @@ -199,15 +194,14 @@ export class PythonResultResolver implements ITestResultResolver { } /** - * Find a test item efficiently using the pre-built maps, with fallback to tree search only if needed. - * This avoids the O(k) search for 99% of cases while still handling edge cases. + * 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 { - // FAST PATH: Try the O(1) lookup first + // Try direct O(1) lookup first const directItem = this.runIdToTestItem.get(keyTemp); if (directItem) { - // VALIDATION: Check if the TestItem is still valid (hasn't been deleted from controller) - // This prevents using stale references + // Validate the item is still in the test tree if (this.isTestItemValid(directItem)) { return directItem; } else { @@ -216,18 +210,16 @@ export class PythonResultResolver implements ITestResultResolver { } } - // FALLBACK: Try vsId mapping + // Try vsId mapping as fallback const vsId = this.runIdToVSid.get(keyTemp); if (vsId) { - // Try to find by VS Code ID directly in the controller - // This is still much faster than full tree traversal + // Search by VS Code ID in the controller let foundItem: TestItem | undefined; this.testController.items.forEach((item) => { if (item.id === vsId) { foundItem = item; return; } - // Check children only if not found at top level if (!foundItem) { item.children.forEach((child) => { if (child.id === vsId) { @@ -238,19 +230,18 @@ export class PythonResultResolver implements ITestResultResolver { }); if (foundItem) { - // Cache for next time to avoid this lookup + // Cache for future lookups this.runIdToTestItem.set(keyTemp, foundItem); return foundItem; } else { - // Clean up stale vsId mapping + // Clean up stale mapping this.runIdToVSid.delete(keyTemp); this.vsIdToRunId.delete(vsId); } } - // LAST RESORT: Only do expensive tree traversal if really needed - // This should rarely happen with proper discovery - console.warn(`Falling back to expensive tree search for test: ${keyTemp}`); + // Last resort: full tree search + console.warn(`Falling back to tree search for test: ${keyTemp}`); const testCases = this.collectAllTestCases(); return testCases.find((item) => item.id === vsId); } @@ -278,11 +269,8 @@ export class PythonResultResolver implements ITestResultResolver { } /** - * Clean up stale references from maps (optional method for external cleanup) - * - * Time Complexity: O(n * depth) where n is the number of cached test items and depth - * is the average tree depth. This is much more efficient than the original O(n*m*k) - * tree rebuilding approach, since it only validates existing cache entries. + * 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[] = []; @@ -454,22 +442,11 @@ export class PythonResultResolver implements ITestResultResolver { /** * Process test execution results and update VS Code's Test Explorer with outcomes. - * - * CURRENT PERFORMANCE ISSUE: For each test result, this method rebuilds the entire test tree - * (O(m) traversal) and then searches through all test items (O(k) search). With parameterized - * tests producing many results, this becomes O(n*m*k) complexity, eventually causing stack overflow. + * Uses efficient lookup methods to handle large numbers of test results. */ public _resolveExecution(payload: ExecutionTestPayload, runInstance: TestRun): void { - // PERFORMANCE ISSUE: This method has O(n*m*k) complexity that causes stack overflow: - // - For each test result (n), we rebuild the entire test tree (m items) - // - Then search through all leaf nodes (k nodes) to find the matching test - // - With parameterized tests, n can be large, making this exponentially slow const rawTestExecData = payload as ExecutionTestPayload; if (rawTestExecData !== undefined && rawTestExecData.result !== undefined) { - // Map which holds the subtest information for each test item. - - // PERFORMANCE FIX: No longer need to rebuild test tree for every result! - // Use efficient lookup methods instead for (const keyTemp of Object.keys(rawTestExecData.result)) { const testItem = rawTestExecData.result[keyTemp]; From 8cb82d2e2181af7a73456258c1ee0e4eb86577e5 Mon Sep 17 00:00:00 2001 From: Eleanor Boyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Thu, 18 Sep 2025 15:12:19 -0700 Subject: [PATCH 3/5] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/client/testing/testController/common/resultResolver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index e7c42eb0d69d..3e2962fc7d76 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -293,7 +293,7 @@ export class PythonResultResolver implements ITestResultResolver { }); if (staleRunIds.length > 0) { - console.log(`Cleaned up ${staleRunIds.length} stale test item references`); + traceVerbose(`Cleaned up ${staleRunIds.length} stale test item references`); } } From 9845d1a2ef8e585252bfb15e3e2e1edd4e432523 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Thu, 18 Sep 2025 15:22:40 -0700 Subject: [PATCH 4/5] fix logging --- src/client/testing/testController/common/resultResolver.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index 3e2962fc7d76..fd46c65f259d 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -25,7 +25,7 @@ import { ITestResultResolver, } from './types'; import { TestProvider } from '../../types'; -import { traceError, traceVerbose } from '../../../logging'; +import { traceError, traceInfo, traceVerbose } from '../../../logging'; import { Testing } from '../../../common/utils/localize'; import { clearAllChildren, createErrorTestItem, getTestCaseNodes } from './testItemUtilities'; import { sendTelemetryEvent } from '../../../telemetry'; @@ -241,7 +241,7 @@ export class PythonResultResolver implements ITestResultResolver { } // Last resort: full tree search - console.warn(`Falling back to tree search for test: ${keyTemp}`); + traceError(`Falling back to tree search for test: ${keyTemp}`); const testCases = this.collectAllTestCases(); return testCases.find((item) => item.id === vsId); } From 5a2fd3ef950ef807d4566768c1b8ace55ac359fb Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Thu, 18 Sep 2025 15:25:31 -0700 Subject: [PATCH 5/5] linting --- src/client/testing/testController/common/resultResolver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index fd46c65f259d..b92e7a870f20 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -25,7 +25,7 @@ import { ITestResultResolver, } from './types'; import { TestProvider } from '../../types'; -import { traceError, traceInfo, traceVerbose } from '../../../logging'; +import { traceError, traceVerbose } from '../../../logging'; import { Testing } from '../../../common/utils/localize'; import { clearAllChildren, createErrorTestItem, getTestCaseNodes } from './testItemUtilities'; import { sendTelemetryEvent } from '../../../telemetry';