From d47be734869b1929fd77c1798d1a3efea1a95d62 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 30 Nov 2025 18:51:30 -0800 Subject: [PATCH 01/10] spec-planning --- .../testController/resultHandlerRefactor.md | 1014 +++++++++++++++++ 1 file changed, 1014 insertions(+) create mode 100644 src/client/testing/testController/resultHandlerRefactor.md diff --git a/src/client/testing/testController/resultHandlerRefactor.md b/src/client/testing/testController/resultHandlerRefactor.md new file mode 100644 index 000000000000..b3614f94b52b --- /dev/null +++ b/src/client/testing/testController/resultHandlerRefactor.md @@ -0,0 +1,1014 @@ +# Result Resolver Refactoring Specification + +## Overview + +Refactor `PythonResultResolver` to separate stateful concerns (ID mappings) from stateless processing logic (payload handling). This improves testability, clarity, and sets the foundation for future project-based architecture. + +## Current Problems + +The `PythonResultResolver` class conflates multiple responsibilities: +1. **Persistent state**: ID mappings between Python test IDs and VS Code TestItem IDs +2. **Discovery processing**: Building TestItem trees from JSON payloads +3. **Execution processing**: Updating TestRun instances from execution results +4. **Coverage processing**: Handling coverage data +5. **Error handling**: Creating error nodes + +This mixing of state and processing makes it: +- Hard to reason about lifecycle and ownership +- Difficult to test in isolation +- Unclear what state persists vs. what is transient +- Prone to state conflicts if multiple operations run concurrently + +## New Design: Separation of Concerns + +### Ownership Model Summary + +**Singleton (One per Extension):** +- `TestDiscoveryHandler` - stateless, shared by all workspaces +- `TestExecutionHandler` - stateless, shared by all workspaces +- `TestCoverageHandler` - stateless, shared by all workspaces + +**Per-Workspace:** +- `PythonResultResolver` - facade that coordinates components +- `TestItemIndex` - stateful, stores ID mappings for this workspace's tests + +**Shared References:** +- `TestController` - one per extension, passed to handlers as parameter +- `TestRun` - one per execution request, passed to handlers as parameter + +**Why This Works:** +- Handlers are pure functions with no instance state → safe to share +- All state is passed as parameters or stored in caller (resolver/index) +- Each workspace gets its own index, but all use the same handler logic + +### State Lifecycle: Where Test ID Mappings Live + +The `TestItemIndex` is **the only stateful component** in the new design. Here's its complete lifecycle: + +#### 1. Creation (Workspace Activation) +```typescript +// In PythonTestController.activate() +const resultResolver = new PythonResultResolver( + this.testController, + testProvider, + workspace.uri +); + +// Inside PythonResultResolver constructor +constructor(testController, testProvider, workspaceUri) { + this.testItemIndex = new TestItemIndex(); // ← CREATED HERE + // Initially empty: no test IDs registered yet +} +``` + +#### 2. Population (Discovery Phase) +```typescript +// Discovery flow: +Python subprocess → DiscoveryAdapter → PythonResultResolver.resolveDiscovery() + → TestDiscoveryHandler.processDiscovery() + +// Inside TestDiscoveryHandler.processDiscovery() +testItemIndex.clear(); // Wipe old mappings + +for each discovered test { + // Create VS Code TestItem + const testItem = testController.createTestItem(test.id_, test.name, uri); + + // Register in index (WRITES STATE) + testItemIndex.registerTestItem( + runId: test.runID, // e.g., "test_file.py::test_example" + vsId: test.id_, // e.g., "test_file.py::test_example" + testItem: testItem // Reference to VS Code TestItem object + ); +} + +// Now index contains: +// runIdToTestItem: { "test_file.py::test_example" → TestItem } +// runIdToVSid: { "test_file.py::test_example" → "test_file.py::test_example" } +// vsIdToRunId: { "test_file.py::test_example" → "test_file.py::test_example" } +``` + +#### 3. Query (Execution Preparation) +```typescript +// In WorkspaceTestAdapter.executeTests() +// User selected some tests to run, need to convert VS Code IDs → Python IDs + +testCaseNodes.forEach((node) => { + // READS STATE from index (via resolver getter) + const runId = resultResolver.vsIdToRunId.get(node.id); + // ↑ This delegates to: testItemIndex.getRunId(node.id) + + if (runId) { + testCaseIds.push(runId); // Send to Python subprocess + } +}); +``` + +#### 4. Lookup (Execution Results Processing) +```typescript +// Execution flow: +Python subprocess → ExecutionAdapter → PythonResultResolver.resolveExecution() + → TestExecutionHandler.processExecution() + +// Inside TestExecutionHandler.processExecution() +for each test result in payload { + const runId = "test_file.py::test_example"; // From Python + + // READS STATE from index + const testItem = testItemIndex.getTestItem(runId, testController); + // ↑ Looks up: runIdToTestItem.get("test_file.py::test_example") → TestItem + + if (testItem) { + runInstance.passed(testItem); // Update VS Code UI + } +} +``` + +#### 5. Cleanup (Periodic or On Demand) +```typescript +// When tests are deleted/modified without full rediscovery +resultResolver.cleanupStaleReferences(); +// ↓ Delegates to: +testItemIndex.cleanupStaleReferences(testController); + +// Removes mappings for TestItems that no longer exist in the tree +``` + +#### 6. Disposal (Workspace Closed) +```typescript +// When workspace is removed or extension deactivates +// PythonResultResolver gets garbage collected +// → testItemIndex gets garbage collected +// → Maps are freed +``` + +### Key Insight: Index is the "Glue" + +``` +┌─────────────────────────────────────────────────────────────┐ +│ DISCOVERY PHASE │ +├─────────────────────────────────────────────────────────────┤ +│ Python: "test_file.py::test_example" │ +│ ↓ │ +│ TestDiscoveryHandler: Creates TestItem │ +│ ↓ │ +│ TestItemIndex: Stores mapping │ +│ runId "test_file.py::test_example" → TestItem instance │ +└─────────────────────────────────────────────────────────────┘ + ↓ + (Index persists) + ↓ +┌─────────────────────────────────────────────────────────────┐ +│ EXECUTION PHASE │ +├─────────────────────────────────────────────────────────────┤ +│ Python: "test_file.py::test_example" succeeded │ +│ ↓ │ +│ TestExecutionHandler: Needs TestItem to update │ +│ ↓ │ +│ TestItemIndex: Retrieves mapping │ +│ runId "test_file.py::test_example" → TestItem instance │ +│ ↓ │ +│ runInstance.passed(testItem) → UI updates ✓ │ +└─────────────────────────────────────────────────────────────┘ +``` + +The index is **persistent state** that bridges discovery and execution, while handlers are **stateless processors** that operate on that state. + +### Component 1: `TestItemIndex` (Stateful - Per Workspace/Adapter) + +**Purpose**: Maintains persistent ID mappings between Python test IDs and VS Code TestItems. + +**Ownership**: **One instance per workspace**. Created by `PythonResultResolver` constructor. + +**Lifecycle**: +- **Created**: When `PythonResultResolver` is instantiated (during workspace activation) +- **Populated**: During discovery - each discovered test registers its mappings +- **Queried**: During execution - to look up TestItems by Python run ID +- **Cleared**: When discovery runs again (fresh start) or workspace is disposed +- **Cleaned**: Periodically to remove stale references to deleted tests + +**State Management**: +```typescript +// Discovery phase - WRITES to index +TestDiscoveryHandler.processDiscovery() { + testItemIndex.clear(); // Start fresh + for each discovered test: + testItemIndex.registerTestItem(test.runID, test.id_, testItem); +} + +// Execution phase - READS from index +TestExecutionHandler.processExecution() { + for each test result: + testItem = testItemIndex.getTestItem(runId); // Lookup! + runInstance.passed/failed/errored(testItem); +} +``` + +**Responsibilities**: +- Store bidirectional mappings: `runId ↔ TestItem`, `runId ↔ vsId` +- Provide efficient lookup methods +- Clean up stale references when tests are removed +- Validate TestItem references are still in the tree + +**Location**: `src/client/testing/testController/common/testItemIndex.ts` + +**Interface**: +```typescript +export class TestItemIndex { + // THE STATE - these maps persist across discovery and execution + private runIdToTestItem: Map; + private runIdToVSid: Map; + private vsIdToRunId: Map; + + constructor(); + + /** + * Register a test item with its Python run ID and VS Code ID + * Called during DISCOVERY to populate the index + */ + registerTestItem(runId: string, vsId: string, testItem: TestItem): void; + + /** + * Get TestItem by Python run ID (with validation and fallback strategies) + * Called during EXECUTION to look up tests + */ + getTestItem(runId: string, testController: TestController): TestItem | undefined; + + /** + * Get Python run ID from VS Code ID + * Called by WorkspaceTestAdapter.executeTests() to convert selected tests to Python IDs + */ + getRunId(vsId: string): string | undefined; + + /** + * Get VS Code ID from Python run ID + */ + getVSId(runId: string): string | undefined; + + /** + * Check if a TestItem reference is still valid in the tree + */ + isTestItemValid(testItem: TestItem, testController: TestController): boolean; + + /** + * Remove all mappings + * Called at the start of discovery to ensure clean state + */ + clear(): void; + + /** + * Clean up stale references that no longer exist in the test tree + * Called after test tree modifications + */ + cleanupStaleReferences(testController: TestController): void; +} +``` + +### Component 2: `TestDiscoveryHandler` (Stateless - Shared) + +**Purpose**: Processes discovery payloads and builds/updates the TestItem tree. + +**Ownership**: **One shared instance** created at the module/service level, reused by all resolvers/adapters. + +**Responsibilities**: +- Parse `DiscoveredTestPayload` and create/update TestItems +- Call `TestItemIndex.registerTestItem()` for each discovered test +- Handle discovery errors and create error nodes +- Populate test tree structure + +**Location**: `src/client/testing/testController/common/testDiscoveryHandler.ts` + +**Interface**: +```typescript +export class TestDiscoveryHandler { + /** + * Process discovery payload and update test tree + * Pure function - no instance state used + */ + processDiscovery( + payload: DiscoveredTestPayload, + testController: TestController, + testItemIndex: TestItemIndex, + workspaceUri: Uri, + testProvider: TestProvider, + token?: CancellationToken + ): void; + + /** + * Create an error node for discovery failures + */ + createErrorNode( + testController: TestController, + workspaceUri: Uri, + message: string, + testProvider: TestProvider + ): TestItem; +} +``` + +### Component 3: `TestExecutionHandler` (Stateless - Shared) + +**Purpose**: Processes execution payloads and updates TestRun instances. + +**Ownership**: **One shared instance** created at the module/service level, reused by all resolvers/adapters. + +**Responsibilities**: +- Parse `ExecutionTestPayload` and update TestRun with results (passed/failed/skipped/errored) +- Look up TestItems using `TestItemIndex` +- Handle subtests (create child TestItems dynamically) +- Process test outcomes and create TestMessages + +**Location**: `src/client/testing/testController/common/testExecutionHandler.ts` + +**Interface**: +```typescript +export class TestExecutionHandler { + /** + * Process execution payload and update test run + * Pure function - no instance state used + * Returns subtest statistics for caller to manage + */ + processExecution( + payload: ExecutionTestPayload, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController + ): Map; + + /** + * Handle a single test result based on outcome + */ + private handleTestOutcome( + runId: string, + testItem: any, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + subtestStats: Map + ): void; +} + +export interface SubtestStats { + passed: number; + failed: number; +} +``` + +### Component 4: `TestCoverageHandler` (Stateless - Shared) + +**Purpose**: Processes coverage payloads and creates coverage objects. + +**Ownership**: **One shared instance** created at the module/service level, reused by all resolvers/adapters. + +**Responsibilities**: +- Parse `CoveragePayload` and create `FileCoverage` objects +- Generate detailed coverage information +- Return coverage data for caller to store/use + +**Location**: `src/client/testing/testController/common/testCoverageHandler.ts` + +**Interface**: +```typescript +export class TestCoverageHandler { + /** + * Process coverage payload + * Pure function - returns coverage data without storing it + */ + processCoverage( + payload: CoveragePayload, + runInstance: TestRun + ): Map; + + /** + * Create FileCoverage object from metrics + */ + private createFileCoverage( + uri: Uri, + metrics: FileCoverageMetrics + ): FileCoverage; + + /** + * Create detailed coverage array for a file + */ + private createDetailedCoverage( + linesCovered: number[], + linesMissed: number[] + ): FileCoverageDetail[]; +} +``` + +### Component 5: `PythonResultResolver` (Adapter/Facade - Maintained for Compatibility) + +**Purpose**: Maintains backward compatibility during transition. Delegates to new components. + +**Ownership**: **One instance per workspace** (current model). References shared handler instances. + +**Responsibilities**: +- Wrap new components to maintain existing API +- Eventually can be deprecated once all callers migrate + +**Location**: `src/client/testing/testController/common/resultResolver.ts` (modified) + +**Interface**: +```typescript +export class PythonResultResolver implements ITestResultResolver { + private testItemIndex: TestItemIndex; // Per-workspace instance + private testController: TestController; // Shared reference + private testProvider: TestProvider; // Configuration + private workspaceUri: Uri; // Configuration + + // References to shared singleton handlers + private static discoveryHandler: TestDiscoveryHandler = new TestDiscoveryHandler(); + private static executionHandler: TestExecutionHandler = new TestExecutionHandler(); + private static coverageHandler: TestCoverageHandler = new TestCoverageHandler(); + + // Expose for backward compatibility (WorkspaceTestAdapter accesses these) + public get runIdToTestItem(): Map { + return this.testItemIndex.runIdToTestItem; + } + public get runIdToVSid(): Map { + return this.testItemIndex.runIdToVSid; + } + public get vsIdToRunId(): Map { + return this.testItemIndex.vsIdToRunId; + } + public subTestStats: Map; + public detailedCoverageMap: Map; + + constructor( + testController: TestController, + testProvider: TestProvider, + workspaceUri: Uri + ) { + this.testController = testController; + this.testProvider = testProvider; + this.workspaceUri = workspaceUri; + this.testItemIndex = new TestItemIndex(); // Per-workspace state + this.subTestStats = new Map(); + this.detailedCoverageMap = new Map(); + } + + public resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void { + PythonResultResolver.discoveryHandler.processDiscovery( + payload, + this.testController, + this.testItemIndex, + this.workspaceUri, + this.testProvider, + token + ); + } + + public resolveExecution(payload: ExecutionTestPayload | CoveragePayload, runInstance: TestRun): void { + if ('coverage' in payload) { + const coverageMap = PythonResultResolver.coverageHandler.processCoverage(payload, runInstance); + this.detailedCoverageMap = coverageMap; + } else { + this.subTestStats = PythonResultResolver.executionHandler.processExecution( + payload, + runInstance, + this.testItemIndex, + this.testController + ); + } + } + + // Delegate cleanup to index + public cleanupStaleReferences(): void { + this.testItemIndex.cleanupStaleReferences(this.testController); + } +} +``` + +## Migration Strategy + +### Phase 1: Extract `TestItemIndex` +**Goal**: Separate ID mapping state from processing logic + +**Steps**: +1. Create `src/client/testing/testController/common/testItemIndex.ts` +2. Move mapping-related methods from `PythonResultResolver`: + - `runIdToTestItem`, `runIdToVSid`, `vsIdToRunId` maps + - `findTestItemByIdEfficient()` + - `isTestItemValid()` + - `cleanupStaleReferences()` +3. Update `PythonResultResolver` to use `TestItemIndex` internally +4. Maintain backward compatibility with existing API +5. Add unit tests for `TestItemIndex` + +**Files Changed**: +- New: `testItemIndex.ts` +- Modified: `resultResolver.ts` + +**Tests**: +- Test ID registration and lookup +- Test stale reference cleanup +- Test validation logic + +### Phase 2: Extract `TestDiscoveryHandler` +**Goal**: Separate discovery processing into stateless handler + +**Steps**: +1. Create `src/client/testing/testController/common/testDiscoveryHandler.ts` +2. Move discovery-related methods from `PythonResultResolver`: + - `_resolveDiscovery()` + - Error node creation logic + - Test tree population logic (from `utils.ts`) +3. Update `PythonResultResolver.resolveDiscovery()` to delegate to handler +4. Add unit tests for `TestDiscoveryHandler` + +**Files Changed**: +- New: `testDiscoveryHandler.ts` +- Modified: `resultResolver.ts` +- Modified: `utils.ts` (move `populateTestTree` to handler) + +**Tests**: +- Test discovery payload processing +- Test error handling +- Test TestItem creation and tree building + +### Phase 3: Extract `TestExecutionHandler` +**Goal**: Separate execution processing into stateless handler + +**Steps**: +1. Create `src/client/testing/testController/common/testExecutionHandler.ts` +2. Move execution-related methods from `PythonResultResolver`: + - `_resolveExecution()` + - `handleTestError()`, `handleTestFailure()`, `handleTestSuccess()`, `handleTestSkipped()` + - `handleSubtestFailure()`, `handleSubtestSuccess()` +3. Update handler to return subtest stats instead of storing them +4. Update `PythonResultResolver.resolveExecution()` to delegate to handler +5. Add unit tests for `TestExecutionHandler` + +**Files Changed**: +- New: `testExecutionHandler.ts` +- Modified: `resultResolver.ts` + +**Tests**: +- Test each outcome type (error, failure, success, skipped) +- Test subtest handling +- Test message creation with tracebacks + +### Phase 4: Extract `TestCoverageHandler` +**Goal**: Separate coverage processing into stateless handler + +**Steps**: +1. Create `src/client/testing/testController/common/testCoverageHandler.ts` +2. Move coverage-related methods from `PythonResultResolver`: + - `_resolveCoverage()` + - Coverage object creation logic +3. Update handler to return coverage data instead of storing it +4. Update `PythonResultResolver.resolveExecution()` to delegate to handler and store results +5. Add unit tests for `TestCoverageHandler` + +**Files Changed**: +- New: `testCoverageHandler.ts` +- Modified: `resultResolver.ts` + +**Tests**: +- Test coverage payload processing +- Test FileCoverage creation +- Test detailed coverage generation + +### Phase 5: Update `PythonResultResolver` to Pure Facade +**Goal**: Simplify resolver to only coordinate components + +**Steps**: +1. Remove all processing logic from `PythonResultResolver` +2. Keep only delegation and backward compatibility methods +3. Update constructor to instantiate handler components +4. Ensure all existing tests still pass + +**Files Changed**: +- Modified: `resultResolver.ts` + +### Phase 6: Direct Migration (Optional - Future) +**Goal**: Update callers to use handlers directly instead of through resolver + +**Steps**: +1. Update `WorkspaceTestAdapter` to use handlers directly +2. Update discovery/execution adapters if needed +3. Eventually deprecate `PythonResultResolver` once all callers migrated + +**Files Changed**: +- Modified: `workspaceTestAdapter.ts` +- Modified: `pytestDiscoveryAdapter.ts`, `unittestDiscoveryAdapter.ts` +- Modified: `pytestExecutionAdapter.ts`, `unittestExecutionAdapter.ts` + +## Benefits + +### Immediate Benefits +1. **Testability**: Each component can be unit tested in isolation with simple inputs/outputs +2. **Clarity**: Clear separation between state (index) and processing (handlers) +3. **Maintainability**: Smaller, focused classes are easier to understand and modify +4. **Type Safety**: Clearer interfaces and types for each concern + +### Future Benefits (Project-based Architecture) +1. **Concurrency**: Stateless handlers can be safely shared across projects +2. **Scalability**: Each project gets its own `TestItemIndex`, handlers are shared +3. **Flexibility**: Easy to add new processing logic without modifying state management +4. **Migration Path**: Clean abstractions make it easier to introduce project adapters + +##################################################### + +# Parts to do later -> + +## Testing Strategy + +### Unit Tests + +**`TestItemIndex`**: +- Test registration and lookup operations +- Test stale reference detection +- Test cleanup operations +- Test edge cases (missing items, invalid references) + +**`TestDiscoveryHandler`**: +- Test parsing valid discovery payloads +- Test error payload handling +- Test tree building with various structures +- Test edge cases (null tests, empty payloads) + +**`TestExecutionHandler`**: +- Test each outcome type (success, failure, error, skip) +- Test subtest creation and statistics +- Test message creation with locations +- Test edge cases (missing items, invalid IDs) + +**`TestCoverageHandler`**: +- Test coverage payload parsing +- Test FileCoverage creation +- Test detailed coverage generation +- Test branch coverage vs. line coverage + +### Integration Tests + +**End-to-end discovery flow**: +- Verify discovery adapters → handlers → TestController updates work correctly +- Verify index is populated correctly during discovery + +**End-to-end execution flow**: +- Verify execution adapters → handlers → TestRun updates work correctly +- Verify index lookups work during execution + +### Regression Tests + +- Run full test suite to ensure no behavioral changes +- Verify all existing discovery/execution scenarios still work +- Verify coverage functionality unchanged + +## Risks and Mitigations + +### Risk: Breaking Changes +**Mitigation**: Maintain `PythonResultResolver` as compatibility facade during transition. All existing callers continue to work unchanged. + +### Risk: Performance Regression +**Mitigation**: Handlers are pure functions with no additional overhead. Index operations remain O(1) lookups. Profile before/after to verify. + +### Risk: Incomplete State Migration +**Mitigation**: Phase 1 focuses entirely on extracting index with full backward compatibility. Each phase is independently testable. + +### Risk: Subtest Stats State Management +**Mitigation**: Return stats from handler rather than storing. Let caller (resolver or adapter) decide how to manage this transient state. + +## Success Criteria + +1. ✅ All existing tests pass without modification +2. ✅ New unit tests achieve >90% coverage for new components +3. ✅ No performance degradation in discovery/execution +4. ✅ Code is more modular and testable +5. ✅ Clear path forward for project-based architecture +6. ✅ No breaking changes to external APIs + +## Information Flow + +### Discovery Flow: From Python Subprocess to Test Tree + +``` +┌─────────────────────────────────────────────────────────────────────────┐ +│ 1. USER ACTION: Refresh Tests / Auto-discovery Trigger │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 2. PythonTestController.refreshTestData() │ +│ - Determines which workspace(s) to refresh │ +│ - Calls refreshSingleWorkspace(uri) or refreshAllWorkspaces() │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 3. PythonTestController.discoverTestsForProvider() │ +│ - Gets WorkspaceTestAdapter for workspace │ +│ - Calls testAdapter.discoverTests(...) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 4. WorkspaceTestAdapter.discoverTests() │ +│ - Calls discoveryAdapter.discoverTests(uri, factory, token) │ +│ - Waits for discovery to complete │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 5. [Pytest|Unittest]DiscoveryAdapter.discoverTests() │ +│ - Sets up named pipe for IPC │ +│ - Spawns Python subprocess with discovery script │ +│ - Subprocess runs: python_files/vscode_pytest/run_pytest_script.py │ +│ or python_files/unittestadapter/discovery.py │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 6. PYTHON SUBPROCESS: Discovery Script │ +│ - Collects tests using pytest/unittest framework │ +│ - Builds DiscoveredTestPayload JSON: │ +│ { │ +│ "status": "success", │ +│ "cwd": "/workspace/path", │ +│ "tests": { │ +│ "rootid": ".", │ +│ "root": "/workspace/path", │ +│ "parents": [...], │ +│ "tests": [ │ +│ { │ +│ "name": "test_example", │ +│ "path": "./test_file.py", │ +│ "type_": "test", │ +│ "id_": "test_file.py::test_example", │ +│ "runID": "test_file.py::test_example", │ +│ "lineno": 10 │ +│ } │ +│ ] │ +│ } │ +│ } │ +│ - Sends JSON over named pipe │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 7. Discovery Adapter: Receives IPC Data │ +│ - Named pipe listener receives JSON chunks │ +│ - Parses complete JSON into DiscoveredTestPayload │ +│ - Calls: resultResolver.resolveDiscovery(payload, token) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 8. PythonResultResolver.resolveDiscovery() [FACADE] │ +│ - Validates payload is not null │ +│ - Delegates to: │ +│ this.discoveryHandler.processDiscovery( │ +│ payload, │ +│ this.testController, │ +│ this.testItemIndex, │ +│ this.workspaceUri, │ +│ this.testProvider, │ +│ token │ +│ ) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 9. TestDiscoveryHandler.processDiscovery() [STATELESS] │ +│ - Checks payload.status for errors │ +│ - If errors: creates error node and adds to TestController │ +│ - If success: processes payload.tests │ +│ - Calls populateTestTree() to build TestItem hierarchy │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 10. TestDiscoveryHandler: Build Test Tree │ +│ - Clears testItemIndex mappings (fresh start) │ +│ - Iterates through discovered tests recursively │ +│ - For each test: │ +│ a. Creates VS Code TestItem: │ +│ testItem = testController.createTestItem( │ +│ id: test.id_, │ +│ label: test.name, │ +│ uri: Uri.file(test.path) │ +│ ) │ +│ b. Sets TestItem properties (range, canResolveChildren, etc.) │ +│ c. Registers in index: │ +│ testItemIndex.registerTestItem( │ +│ runId: test.runID, │ +│ vsId: test.id_, │ +│ testItem: testItem │ +│ ) │ +│ d. Adds TestItem to parent or TestController.items │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 11. TestItemIndex.registerTestItem() [STATEFUL] │ +│ - Stores mappings: │ +│ runIdToTestItem.set(runId, testItem) │ +│ runIdToVSid.set(runId, vsId) │ +│ vsIdToRunId.set(vsId, runId) │ +│ - These mappings persist for execution phase │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 12. VS CODE TEST EXPLORER: Tree Updated │ +│ - TestController.items now contains full test hierarchy │ +│ - Test Explorer UI refreshes to show discovered tests │ +│ - Tests are ready to be run │ +└─────────────────────────────────────────────────────────────────────────┘ + + +### Execution Flow: From Run Request to Result Updates + +┌─────────────────────────────────────────────────────────────────────────┐ +│ 1. USER ACTION: Run/Debug Tests │ +│ - User clicks run icon, uses command palette, or keyboard shortcut │ +│ - VS Code creates TestRunRequest with selected TestItems │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 2. PythonTestController.runTests(request, token) │ +│ - Gets workspaces that contain selected tests │ +│ - Creates TestRun instance from TestController │ +│ - Calls runTestsForWorkspace() for each workspace │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 3. PythonTestController.runTestsForWorkspace() │ +│ - Filters TestItems belonging to this workspace │ +│ - Gets WorkspaceTestAdapter for workspace │ +│ - Calls testAdapter.executeTests(...) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 4. WorkspaceTestAdapter.executeTests() │ +│ - Collects all test case nodes from selected items │ +│ - For each test, looks up Python runID: │ +│ runId = resultResolver.vsIdToRunId.get(node.id) │ +│ - Calls runInstance.started(node) for each test │ +│ - Calls executionAdapter.runTests(uri, testCaseIds, ...) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 5. [Pytest|Unittest]ExecutionAdapter.runTests() │ +│ - Sets up named pipe for IPC │ +│ - Spawns Python subprocess with execution script │ +│ - Subprocess runs: python_files/vscode_pytest/run_pytest_script.py │ +│ or python_files/unittestadapter/execution.py │ +│ - Passes test IDs as arguments │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 6. PYTHON SUBPROCESS: Execution Script │ +│ - Runs selected tests using pytest/unittest framework │ +│ - Captures results in real-time │ +│ - For each test result, builds ExecutionTestPayload: │ +│ { │ +│ "result": { │ +│ "test_file.py::test_example": { │ +│ "test": "test_file.py::test_example", │ +│ "outcome": "success", // or "failure", "error", "skipped" │ +│ "message": "...", │ +│ "traceback": "...", │ +│ "subtest": null │ +│ } │ +│ } │ +│ } │ +│ - Sends JSON over named pipe (can send multiple payloads) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 7. Execution Adapter: Receives IPC Data (Streaming) │ +│ - Named pipe listener receives JSON chunks as tests complete │ +│ - For each complete JSON payload: │ +│ - Parses into ExecutionTestPayload or CoveragePayload │ +│ - Calls: resultResolver.resolveExecution(payload, runInstance) │ +│ - Updates happen in real-time as tests finish │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 8. PythonResultResolver.resolveExecution() [FACADE] │ +│ - Checks payload type: │ +│ if ('coverage' in payload): │ +│ coverageMap = coverageHandler.processCoverage(payload, run) │ +│ this.detailedCoverageMap = coverageMap │ +│ else: │ +│ subtestStats = executionHandler.processExecution( │ +│ payload, runInstance, testItemIndex, testController │ +│ ) │ +│ this.subTestStats = subtestStats │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 9a. TestExecutionHandler.processExecution() [STATELESS] │ +│ - Iterates through payload.result entries │ +│ - For each test result: │ +│ a. Looks up TestItem using index: │ +│ testItem = testItemIndex.getTestItem(runId, testController) │ +│ b. Routes to outcome-specific handler based on outcome: │ +│ - "success" → handleTestSuccess() │ +│ - "failure" → handleTestFailure() │ +│ - "error" → handleTestError() │ +│ - "skipped" → handleTestSkipped() │ +│ - "subtest-success" → handleSubtestSuccess() │ +│ - "subtest-failure" → handleSubtestFailure() │ +│ - Returns Map of subtest statistics │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 9b. TestCoverageHandler.processCoverage() [STATELESS] │ +│ - Iterates through payload.result (file paths → metrics) │ +│ - For each file: │ +│ a. Creates FileCoverage object with line/branch counts │ +│ b. Calls runInstance.addCoverage(fileCoverage) │ +│ c. Builds detailed coverage array (StatementCoverage objects) │ +│ - Returns Map │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 10. TestExecutionHandler: Update Test Results │ +│ - handleTestSuccess(runId, runInstance): │ +│ testItem = index.getTestItem(runId) │ +│ runInstance.passed(testItem) │ +│ │ +│ - handleTestFailure(runId, testData, runInstance): │ +│ testItem = index.getTestItem(runId) │ +│ message = new TestMessage(error text + traceback) │ +│ message.location = new Location(testItem.uri, testItem.range) │ +│ runInstance.failed(testItem, message) │ +│ │ +│ - handleTestError(runId, testData, runInstance): │ +│ testItem = index.getTestItem(runId) │ +│ message = new TestMessage(error details) │ +│ runInstance.errored(testItem, message) │ +│ │ +│ - handleTestSkipped(runId, runInstance): │ +│ testItem = index.getTestItem(runId) │ +│ runInstance.skipped(testItem) │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 11. TestExecutionHandler: Handle Subtests (if applicable) │ +│ - handleSubtestFailure/Success(runId, testData, runInstance): │ +│ parentTestItem = index.getTestItem(parentRunId) │ +│ subtestItem = testController.createTestItem(subtestId, ...) │ +│ parentTestItem.children.add(subtestItem) │ +│ runInstance.started(subtestItem) │ +│ runInstance.passed/failed(subtestItem, message) │ +│ - Updates subtest statistics map and returns to caller │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 12. TestItemIndex: Lookup Operations [STATEFUL] │ +│ - getTestItem(runId, testController): │ +│ 1. Try direct lookup: runIdToTestItem.get(runId) │ +│ 2. Validate item is still in tree: isTestItemValid() │ +│ 3. If stale, try vsId mapping: runIdToVSid.get(runId) │ +│ 4. Search tree using vsId │ +│ 5. Fall back to full tree search if needed │ +│ - Returns TestItem or undefined │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 13. VS CODE TEST EXPLORER: Results Updated in Real-Time │ +│ - runInstance.passed/failed/errored/skipped() calls update UI │ +│ - Test items show green checkmarks, red X's, warnings │ +│ - Error messages and tracebacks display in peek view │ +│ - Coverage decorations appear in editor (if coverage enabled) │ +│ - Duration and output appear in test results panel │ +└─────────────────────────────────────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────────────────────────────────────┐ +│ 14. Execution Complete │ +│ - Python subprocess exits │ +│ - Execution adapter resolves promise │ +│ - WorkspaceTestAdapter.executeTests() returns │ +│ - PythonTestController calls runInstance.end() │ +│ - Final telemetry sent │ +└─────────────────────────────────────────────────────────────────────────┘ +``` + +### Key Observations + +**State Management**: +- `TestItemIndex` maintains persistent mappings created during discovery +- These mappings are reused during execution for efficient lookups +- Handlers are stateless - they don't store data between calls +- Transient state (subtest stats, coverage map) returned to caller for storage + +**Data Flow Direction**: +- Discovery: Python → Adapter → Resolver → Handler → TestController (builds tree) +- Execution: Python → Adapter → Resolver → Handler → TestRun (updates results) +- Index: Populated during discovery, queried during execution + +**Separation of Concerns**: +- Adapters: IPC and subprocess management +- Resolver: Coordination and backward compatibility +- Handlers: Pure processing logic (payload → actions) +- Index: ID mapping and lookup optimization + +**Real-time Updates**: +- Execution results stream over IPC as tests complete +- Each payload is processed immediately +- UI updates happen incrementally, not in batch + +## Timeline Estimate + +- Phase 1 (TestItemIndex): 1-2 days +- Phase 2 (TestDiscoveryHandler): 2-3 days +- Phase 3 (TestExecutionHandler): 2-3 days +- Phase 4 (TestCoverageHandler): 1-2 days +- Phase 5 (Facade cleanup): 1 day +- Testing and refinement: 2-3 days + +**Total: ~2 weeks** for complete refactor with comprehensive testing From 1b4f4c8e75e4a97c812ab7ba517b752b0b1ab99a Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 30 Nov 2025 19:46:45 -0800 Subject: [PATCH 02/10] refactor resultResolver --- .../testController/common/resultResolver.ts | 470 ++---------------- .../common/testCoverageHandler.ts | 93 ++++ .../common/testDiscoveryHandler.ts | 104 ++++ .../common/testExecutionHandler.ts | 243 +++++++++ .../testController/common/testItemIndex.ts | 202 ++++++++ 5 files changed, 695 insertions(+), 417 deletions(-) create mode 100644 src/client/testing/testController/common/testCoverageHandler.ts create mode 100644 src/client/testing/testController/common/testDiscoveryHandler.ts create mode 100644 src/client/testing/testController/common/testExecutionHandler.ts create mode 100644 src/client/testing/testController/common/testItemIndex.ts diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index b92e7a870f20..a1ed133e11e6 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -1,48 +1,28 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { - CancellationToken, - TestController, - TestItem, - Uri, - TestMessage, - Location, - TestRun, - MarkdownString, - TestCoverageCount, - FileCoverage, - FileCoverageDetail, - StatementCoverage, - Range, -} from 'vscode'; -import * as util from 'util'; -import { - CoveragePayload, - DiscoveredTestPayload, - ExecutionTestPayload, - FileCoverageMetrics, - ITestResultResolver, -} from './types'; +import { CancellationToken, TestController, TestItem, Uri, TestRun, FileCoverageDetail } from 'vscode'; +import { CoveragePayload, DiscoveredTestPayload, ExecutionTestPayload, ITestResultResolver } from './types'; import { TestProvider } from '../../types'; -import { traceError, traceVerbose } from '../../../logging'; -import { Testing } from '../../../common/utils/localize'; -import { clearAllChildren, createErrorTestItem, getTestCaseNodes } from './testItemUtilities'; +import { traceVerbose } from '../../../logging'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; -import { splitLines } from '../../../common/stringUtils'; -import { buildErrorNodeOptions, populateTestTree, splitTestNameWithRegex } from './utils'; +import { TestItemIndex } from './testItemIndex'; +import { TestDiscoveryHandler } from './testDiscoveryHandler'; +import { TestExecutionHandler } from './testExecutionHandler'; +import { TestCoverageHandler } from './testCoverageHandler'; export class PythonResultResolver implements ITestResultResolver { testController: TestController; testProvider: TestProvider; - public runIdToTestItem: Map; + private testItemIndex: TestItemIndex; - public runIdToVSid: Map; - - public vsIdToRunId: Map; + // Shared singleton handlers + private static discoveryHandler: TestDiscoveryHandler = new TestDiscoveryHandler(); + private static executionHandler: TestExecutionHandler = new TestExecutionHandler(); + private static coverageHandler: TestCoverageHandler = new TestCoverageHandler(); public subTestStats: Map = new Map(); @@ -51,420 +31,76 @@ export class PythonResultResolver implements ITestResultResolver { constructor(testController: TestController, testProvider: TestProvider, private workspaceUri: Uri) { this.testController = testController; this.testProvider = testProvider; - - this.runIdToTestItem = new Map(); - this.runIdToVSid = new Map(); - this.vsIdToRunId = new Map(); + this.testItemIndex = new TestItemIndex(); } - public resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void { - if (!payload) { - // No test data is available - } else { - this._resolveDiscovery(payload as DiscoveredTestPayload, token); - } + // Expose for backward compatibility (WorkspaceTestAdapter accesses these) + public get runIdToTestItem(): Map { + return this.testItemIndex.runIdToTestItemMap; } - public _resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void { - const workspacePath = this.workspaceUri.fsPath; - const rawTestData = payload as DiscoveredTestPayload; - // Check if there were any errors in the discovery process. - if (rawTestData.status === 'error') { - const testingErrorConst = - this.testProvider === 'pytest' ? Testing.errorPytestDiscovery : Testing.errorUnittestDiscovery; - const { error } = rawTestData; - traceError(testingErrorConst, 'for workspace: ', workspacePath, '\r\n', error?.join('\r\n\r\n') ?? ''); - - let errorNode = this.testController.items.get(`DiscoveryError:${workspacePath}`); - const message = util.format( - `${testingErrorConst} ${Testing.seePythonOutput}\r\n`, - error?.join('\r\n\r\n') ?? '', - ); - - if (errorNode === undefined) { - const options = buildErrorNodeOptions(this.workspaceUri, message, this.testProvider); - errorNode = createErrorTestItem(this.testController, options); - this.testController.items.add(errorNode); - } - const errorNodeLabel: MarkdownString = new MarkdownString( - `[Show output](command:python.viewOutput) to view error logs`, - ); - errorNodeLabel.isTrusted = true; - errorNode.error = errorNodeLabel; - } else { - // remove error node only if no errors exist. - this.testController.items.delete(`DiscoveryError:${workspacePath}`); - } - if (rawTestData.tests || rawTestData.tests === null) { - // 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(); + public get runIdToVSid(): Map { + return this.testItemIndex.runIdToVSidMap; + } - // 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); - } + public get vsIdToRunId(): Map { + return this.testItemIndex.vsIdToRunIdMap; + } + public resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void { + PythonResultResolver.discoveryHandler.processDiscovery( + payload, + this.testController, + this.testItemIndex, + this.workspaceUri, + this.testProvider, + token, + ); sendTelemetryEvent(EventName.UNITTEST_DISCOVERY_DONE, undefined, { tool: this.testProvider, failed: false, }); } + public _resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void { + // Delegate to the public method for backward compatibility + this.resolveDiscovery(payload, token); + } + public resolveExecution(payload: ExecutionTestPayload | CoveragePayload, runInstance: TestRun): void { if ('coverage' in payload) { // coverage data is sent once per connection traceVerbose('Coverage data received.'); - this._resolveCoverage(payload as CoveragePayload, runInstance); + this.detailedCoverageMap = PythonResultResolver.coverageHandler.processCoverage( + payload as CoveragePayload, + runInstance, + ); } else { - this._resolveExecution(payload as ExecutionTestPayload, runInstance); - } - } - - public _resolveCoverage(payload: CoveragePayload, runInstance: TestRun): void { - if (payload.result === undefined) { - return; - } - for (const [key, value] of Object.entries(payload.result)) { - const fileNameStr = key; - const fileCoverageMetrics: FileCoverageMetrics = value; - const linesCovered = fileCoverageMetrics.lines_covered ? fileCoverageMetrics.lines_covered : []; // undefined if no lines covered - const linesMissed = fileCoverageMetrics.lines_missed ? fileCoverageMetrics.lines_missed : []; // undefined if no lines missed - const executedBranches = fileCoverageMetrics.executed_branches; - const totalBranches = fileCoverageMetrics.total_branches; - - const lineCoverageCount = new TestCoverageCount( - linesCovered.length, - linesCovered.length + linesMissed.length, + this.subTestStats = PythonResultResolver.executionHandler.processExecution( + payload as ExecutionTestPayload, + runInstance, + this.testItemIndex, + this.testController, ); - let fileCoverage: FileCoverage; - const uri = Uri.file(fileNameStr); - if (totalBranches === -1) { - // branch coverage was not enabled and should not be displayed - fileCoverage = new FileCoverage(uri, lineCoverageCount); - } else { - const branchCoverageCount = new TestCoverageCount(executedBranches, totalBranches); - fileCoverage = new FileCoverage(uri, lineCoverageCount, branchCoverageCount); - } - runInstance.addCoverage(fileCoverage); - - // create detailed coverage array for each file (only line coverage on detailed, not branch) - const detailedCoverageArray: FileCoverageDetail[] = []; - // go through all covered lines, create new StatementCoverage, and add to detailedCoverageArray - for (const line of linesCovered) { - // line is 1-indexed, so we need to subtract 1 to get the 0-indexed line number - // true value means line is covered - const statementCoverage = new StatementCoverage( - true, - new Range(line - 1, 0, line - 1, Number.MAX_SAFE_INTEGER), - ); - detailedCoverageArray.push(statementCoverage); - } - for (const line of linesMissed) { - // line is 1-indexed, so we need to subtract 1 to get the 0-indexed line number - // false value means line is NOT covered - const statementCoverage = new StatementCoverage( - false, - new Range(line - 1, 0, line - 1, Number.MAX_SAFE_INTEGER), - ); - detailedCoverageArray.push(statementCoverage); - } - - this.detailedCoverageMap.set(uri.fsPath, detailedCoverageArray); } } - /** - * 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); + public _resolveExecution(payload: ExecutionTestPayload, runInstance: TestRun): void { + // Delegate to the public method for backward compatibility + this.resolveExecution(payload, runInstance); } - /** - * 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; + public _resolveCoverage(payload: CoveragePayload, runInstance: TestRun): void { + // Delegate to the public method for backward compatibility + this.resolveExecution(payload, runInstance); } /** * Clean up stale test item references from the cache maps. * Validates cached items and removes any that are no longer in the test tree. + * Delegates to TestItemIndex. */ 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) { - for (const keyTemp of Object.keys(rawTestExecData.result)) { - const testItem = rawTestExecData.result[keyTemp]; - - // Delegate to specific outcome handlers using efficient lookups - if (testItem.outcome === 'error') { - this.handleTestError(keyTemp, testItem, runInstance); - } else if (testItem.outcome === 'failure' || testItem.outcome === 'passed-unexpected') { - this.handleTestFailure(keyTemp, testItem, runInstance); - } else if (testItem.outcome === 'success' || testItem.outcome === 'expected-failure') { - this.handleTestSuccess(keyTemp, runInstance); - } else if (testItem.outcome === 'skipped') { - this.handleTestSkipped(keyTemp, runInstance); - } else if (testItem.outcome === 'subtest-failure') { - this.handleSubtestFailure(keyTemp, testItem, runInstance); - } else if (testItem.outcome === 'subtest-success') { - this.handleSubtestSuccess(keyTemp, runInstance); - } - } - } + this.testItemIndex.cleanupStaleReferences(this.testController); } } diff --git a/src/client/testing/testController/common/testCoverageHandler.ts b/src/client/testing/testController/common/testCoverageHandler.ts new file mode 100644 index 000000000000..81ec80579730 --- /dev/null +++ b/src/client/testing/testController/common/testCoverageHandler.ts @@ -0,0 +1,93 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestRun, Uri, TestCoverageCount, FileCoverage, FileCoverageDetail, StatementCoverage, Range } from 'vscode'; +import { CoveragePayload, FileCoverageMetrics } from './types'; + +/** + * Stateless handler for processing coverage payloads and creating coverage objects. + * This handler is shared across all workspaces and contains no instance state. + */ +export class TestCoverageHandler { + /** + * Process coverage payload + * Pure function - returns coverage data without storing it + */ + public processCoverage(payload: CoveragePayload, runInstance: TestRun): Map { + const detailedCoverageMap = new Map(); + + if (payload.result === undefined) { + return detailedCoverageMap; + } + + for (const [key, value] of Object.entries(payload.result)) { + const fileNameStr = key; + const fileCoverageMetrics: FileCoverageMetrics = value; + + // Create FileCoverage object and add to run instance + const fileCoverage = this.createFileCoverage(Uri.file(fileNameStr), fileCoverageMetrics); + runInstance.addCoverage(fileCoverage); + + // Create detailed coverage array for this file + const detailedCoverage = this.createDetailedCoverage( + fileCoverageMetrics.lines_covered ?? [], + fileCoverageMetrics.lines_missed ?? [], + ); + detailedCoverageMap.set(Uri.file(fileNameStr).fsPath, detailedCoverage); + } + + return detailedCoverageMap; + } + + /** + * Create FileCoverage object from metrics + */ + private createFileCoverage(uri: Uri, metrics: FileCoverageMetrics): FileCoverage { + const linesCovered = metrics.lines_covered ?? []; + const linesMissed = metrics.lines_missed ?? []; + const executedBranches = metrics.executed_branches; + const totalBranches = metrics.total_branches; + + const lineCoverageCount = new TestCoverageCount(linesCovered.length, linesCovered.length + linesMissed.length); + + if (totalBranches === -1) { + // branch coverage was not enabled and should not be displayed + return new FileCoverage(uri, lineCoverageCount); + } else { + const branchCoverageCount = new TestCoverageCount(executedBranches, totalBranches); + return new FileCoverage(uri, lineCoverageCount, branchCoverageCount); + } + } + + /** + * Create detailed coverage array for a file + * Only line coverage on detailed, not branch coverage + */ + private createDetailedCoverage(linesCovered: number[], linesMissed: number[]): FileCoverageDetail[] { + const detailedCoverageArray: FileCoverageDetail[] = []; + + // Add covered lines + for (const line of linesCovered) { + // line is 1-indexed, so we need to subtract 1 to get the 0-indexed line number + // true value means line is covered + const statementCoverage = new StatementCoverage( + true, + new Range(line - 1, 0, line - 1, Number.MAX_SAFE_INTEGER), + ); + detailedCoverageArray.push(statementCoverage); + } + + // Add missed lines + for (const line of linesMissed) { + // line is 1-indexed, so we need to subtract 1 to get the 0-indexed line number + // false value means line is NOT covered + const statementCoverage = new StatementCoverage( + false, + new Range(line - 1, 0, line - 1, Number.MAX_SAFE_INTEGER), + ); + detailedCoverageArray.push(statementCoverage); + } + + return detailedCoverageArray; + } +} diff --git a/src/client/testing/testController/common/testDiscoveryHandler.ts b/src/client/testing/testController/common/testDiscoveryHandler.ts new file mode 100644 index 000000000000..50f4fa71406a --- /dev/null +++ b/src/client/testing/testController/common/testDiscoveryHandler.ts @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { CancellationToken, TestController, Uri, MarkdownString } from 'vscode'; +import * as util from 'util'; +import { DiscoveredTestPayload } from './types'; +import { TestProvider } from '../../types'; +import { traceError } from '../../../logging'; +import { Testing } from '../../../common/utils/localize'; +import { createErrorTestItem } from './testItemUtilities'; +import { buildErrorNodeOptions, populateTestTree } from './utils'; +import { TestItemIndex } from './testItemIndex'; + +/** + * Stateless handler for processing discovery payloads and building/updating the TestItem tree. + * This handler is shared across all workspaces and contains no instance state. + */ +export class TestDiscoveryHandler { + /** + * Process discovery payload and update test tree + * Pure function - no instance state used + */ + public processDiscovery( + payload: DiscoveredTestPayload, + testController: TestController, + testItemIndex: TestItemIndex, + workspaceUri: Uri, + testProvider: TestProvider, + token?: CancellationToken, + ): void { + if (!payload) { + // No test data is available + return; + } + + const workspacePath = workspaceUri.fsPath; + const rawTestData = payload as DiscoveredTestPayload; + + // Check if there were any errors in the discovery process. + if (rawTestData.status === 'error') { + this.createErrorNode(testController, workspaceUri, rawTestData.error, testProvider); + } else { + // remove error node only if no errors exist. + testController.items.delete(`DiscoveryError:${workspacePath}`); + } + + if (rawTestData.tests || rawTestData.tests === null) { + // 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 + testItemIndex.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. + // Note: populateTestTree will call testItemIndex.registerTestItem() for each discovered test + populateTestTree( + testController, + rawTestData.tests, + undefined, + { + runIdToTestItem: testItemIndex.runIdToTestItemMap, + runIdToVSid: testItemIndex.runIdToVSidMap, + vsIdToRunId: testItemIndex.vsIdToRunIdMap, + } as any, + token, + ); + } + } + + /** + * Create an error node for discovery failures + */ + public createErrorNode( + testController: TestController, + workspaceUri: Uri, + error: string[] | undefined, + testProvider: TestProvider, + ): void { + const workspacePath = workspaceUri.fsPath; + const testingErrorConst = + testProvider === 'pytest' ? Testing.errorPytestDiscovery : Testing.errorUnittestDiscovery; + + traceError(testingErrorConst, 'for workspace: ', workspacePath, '\r\n', error?.join('\r\n\r\n') ?? ''); + + let errorNode = testController.items.get(`DiscoveryError:${workspacePath}`); + const message = util.format( + `${testingErrorConst} ${Testing.seePythonOutput}\r\n`, + error?.join('\r\n\r\n') ?? '', + ); + + if (errorNode === undefined) { + const options = buildErrorNodeOptions(workspaceUri, message, testProvider); + errorNode = createErrorTestItem(testController, options); + testController.items.add(errorNode); + } + + const errorNodeLabel: MarkdownString = new MarkdownString( + `[Show output](command:python.viewOutput) to view error logs`, + ); + errorNodeLabel.isTrusted = true; + errorNode.error = errorNodeLabel; + } +} diff --git a/src/client/testing/testController/common/testExecutionHandler.ts b/src/client/testing/testController/common/testExecutionHandler.ts new file mode 100644 index 000000000000..d524d9bb48ab --- /dev/null +++ b/src/client/testing/testController/common/testExecutionHandler.ts @@ -0,0 +1,243 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestController, TestRun, TestMessage, Location } from 'vscode'; +import { ExecutionTestPayload } from './types'; +import { TestItemIndex } from './testItemIndex'; +import { splitLines } from '../../../common/stringUtils'; +import { splitTestNameWithRegex } from './utils'; +import { clearAllChildren } from './testItemUtilities'; + +export interface SubtestStats { + passed: number; + failed: number; +} + +/** + * Stateless handler for processing execution payloads and updating TestRun instances. + * This handler is shared across all workspaces and contains no instance state. + */ +export class TestExecutionHandler { + /** + * Process execution payload and update test run + * Pure function - no instance state used + * Returns subtest statistics for caller to manage + */ + public processExecution( + payload: ExecutionTestPayload, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + ): Map { + const subtestStats = new Map(); + const rawTestExecData = payload as ExecutionTestPayload; + + if (rawTestExecData !== undefined && rawTestExecData.result !== undefined) { + for (const keyTemp of Object.keys(rawTestExecData.result)) { + const testItem = rawTestExecData.result[keyTemp]; + + // Delegate to specific outcome handlers + this.handleTestOutcome(keyTemp, testItem, runInstance, testItemIndex, testController, subtestStats); + } + } + + return subtestStats; + } + + /** + * Handle a single test result based on outcome + */ + private handleTestOutcome( + runId: string, + testItem: any, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + subtestStats: Map, + ): void { + if (testItem.outcome === 'error') { + this.handleTestError(runId, testItem, runInstance, testItemIndex, testController); + } else if (testItem.outcome === 'failure' || testItem.outcome === 'passed-unexpected') { + this.handleTestFailure(runId, testItem, runInstance, testItemIndex, testController); + } else if (testItem.outcome === 'success' || testItem.outcome === 'expected-failure') { + this.handleTestSuccess(runId, runInstance, testItemIndex, testController); + } else if (testItem.outcome === 'skipped') { + this.handleTestSkipped(runId, runInstance, testItemIndex, testController); + } else if (testItem.outcome === 'subtest-failure') { + this.handleSubtestFailure(runId, testItem, runInstance, testItemIndex, testController, subtestStats); + } else if (testItem.outcome === 'subtest-success') { + this.handleSubtestSuccess(runId, runInstance, testItemIndex, testController, subtestStats); + } + } + + /** + * Handle test items that errored during execution + */ + private handleTestError( + runId: string, + testItem: any, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + ): 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 = testItemIndex.getTestItem(runId, testController); + + 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( + runId: string, + testItem: any, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + ): 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 = testItemIndex.getTestItem(runId, testController); + + 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( + runId: string, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + ): void { + const foundItem = testItemIndex.getTestItem(runId, testController); + + if (foundItem !== undefined && foundItem.uri) { + runInstance.passed(foundItem); + } + } + + /** + * Handle test items that were skipped during execution + */ + private handleTestSkipped( + runId: string, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + ): void { + const foundItem = testItemIndex.getTestItem(runId, testController); + + if (foundItem !== undefined && foundItem.uri) { + runInstance.skipped(foundItem); + } + } + + /** + * Handle subtest failures + */ + private handleSubtestFailure( + runId: string, + testItem: any, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + subtestStats: Map, + ): void { + const [parentTestCaseId, subtestId] = splitTestNameWithRegex(runId); + const parentTestItem = testItemIndex.getTestItem(parentTestCaseId, testController); + + if (parentTestItem) { + const stats = subtestStats.get(parentTestCaseId); + if (stats) { + stats.failed += 1; + } else { + subtestStats.set(parentTestCaseId, { + failed: 1, + passed: 0, + }); + clearAllChildren(parentTestItem); + } + + const subTestItem = 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( + runId: string, + runInstance: TestRun, + testItemIndex: TestItemIndex, + testController: TestController, + subtestStats: Map, + ): void { + const [parentTestCaseId, subtestId] = splitTestNameWithRegex(runId); + const parentTestItem = testItemIndex.getTestItem(parentTestCaseId, testController); + + if (parentTestItem) { + const stats = subtestStats.get(parentTestCaseId); + if (stats) { + stats.passed += 1; + } else { + subtestStats.set(parentTestCaseId, { failed: 0, passed: 1 }); + clearAllChildren(parentTestItem); + } + + const subTestItem = 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'); + } + } +} diff --git a/src/client/testing/testController/common/testItemIndex.ts b/src/client/testing/testController/common/testItemIndex.ts new file mode 100644 index 000000000000..25a5b8e55db7 --- /dev/null +++ b/src/client/testing/testController/common/testItemIndex.ts @@ -0,0 +1,202 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestController, TestItem } from 'vscode'; +import { traceError, traceVerbose } from '../../../logging'; +import { getTestCaseNodes } from './testItemUtilities'; + +/** + * Maintains persistent ID mappings between Python test IDs and VS Code TestItems. + * This is a stateful component that bridges discovery and execution phases. + * + * Lifecycle: + * - Created: When PythonResultResolver is instantiated (during workspace activation) + * - Populated: During discovery - each discovered test registers its mappings + * - Queried: During execution - to look up TestItems by Python run ID + * - Cleared: When discovery runs again (fresh start) or workspace is disposed + * - Cleaned: Periodically to remove stale references to deleted tests + */ +export class TestItemIndex { + // THE STATE - these maps persist across discovery and execution + private runIdToTestItem: Map; + private runIdToVSid: Map; + private vsIdToRunId: Map; + + constructor() { + this.runIdToTestItem = new Map(); + this.runIdToVSid = new Map(); + this.vsIdToRunId = new Map(); + } + + /** + * Register a test item with its Python run ID and VS Code ID + * Called during DISCOVERY to populate the index + */ + public registerTestItem(runId: string, vsId: string, testItem: TestItem): void { + this.runIdToTestItem.set(runId, testItem); + this.runIdToVSid.set(runId, vsId); + this.vsIdToRunId.set(vsId, runId); + } + + /** + * Get TestItem by Python run ID (with validation and fallback strategies) + * Called during EXECUTION to look up tests + * + * Uses a three-tier approach: + * 1. Direct O(1) lookup in runIdToTestItem map + * 2. If stale, try vsId mapping and search by VS Code ID + * 3. Last resort: full tree search + */ + public getTestItem(runId: string, testController: TestController): TestItem | undefined { + // Try direct O(1) lookup first + const directItem = this.runIdToTestItem.get(runId); + if (directItem) { + // Validate the item is still in the test tree + if (this.isTestItemValid(directItem, testController)) { + return directItem; + } else { + // Clean up stale reference + this.runIdToTestItem.delete(runId); + } + } + + // Try vsId mapping as fallback + const vsId = this.runIdToVSid.get(runId); + if (vsId) { + // Search by VS Code ID in the controller + let foundItem: TestItem | undefined; + 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(runId, foundItem); + return foundItem; + } else { + // Clean up stale mapping + this.runIdToVSid.delete(runId); + this.vsIdToRunId.delete(vsId); + } + } + + // Last resort: full tree search + traceError(`Falling back to tree search for test: ${runId}`); + const testCases = this.collectAllTestCases(testController); + return testCases.find((item) => item.id === vsId); + } + + /** + * Get Python run ID from VS Code ID + * Called by WorkspaceTestAdapter.executeTests() to convert selected tests to Python IDs + */ + public getRunId(vsId: string): string | undefined { + return this.vsIdToRunId.get(vsId); + } + + /** + * Get VS Code ID from Python run ID + */ + public getVSId(runId: string): string | undefined { + return this.runIdToVSid.get(runId); + } + + /** + * Check if a TestItem reference is still valid in the 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. + */ + public isTestItemValid(testItem: TestItem, testController: TestController): 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 testController.items.get(current.id) === current; + } + + // If no parent chain, check if it's directly in the controller + return testController.items.get(testItem.id) === testItem; + } + + /** + * Remove all mappings + * Called at the start of discovery to ensure clean state + */ + public clear(): void { + this.runIdToTestItem.clear(); + this.runIdToVSid.clear(); + this.vsIdToRunId.clear(); + } + + /** + * Clean up stale references that no longer exist in the test tree + * Called after test tree modifications + */ + public cleanupStaleReferences(testController: TestController): void { + const staleRunIds: string[] = []; + + // Check all runId->TestItem mappings + this.runIdToTestItem.forEach((testItem, runId) => { + if (!this.isTestItemValid(testItem, testController)) { + 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`); + } + } + + /** + * Collect all test case items from the test controller tree. + * Note: This performs full tree traversal - use cached lookups when possible. + */ + private collectAllTestCases(testController: TestController): TestItem[] { + const testCases: TestItem[] = []; + + testController.items.forEach((i) => { + const tempArr: TestItem[] = getTestCaseNodes(i); + testCases.push(...tempArr); + }); + + return testCases; + } + + // Expose maps for backward compatibility (read-only access) + public get runIdToTestItemMap(): Map { + return this.runIdToTestItem; + } + + public get runIdToVSidMap(): Map { + return this.runIdToVSid; + } + + public get vsIdToRunIdMap(): Map { + return this.vsIdToRunId; + } +} From 5a831e31e8f96fc842f4d7517211fbbc6a645a22 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 30 Nov 2025 20:20:54 -0800 Subject: [PATCH 03/10] add unittest for new objects --- .../testing-workflow.instructions.md | 579 ++++++++++++++++ .../testController/common/resultResolver.ts | 1 + .../common/testCoverageHandler.unit.test.ts | 502 ++++++++++++++ .../common/testDiscoveryHandler.unit.test.ts | 517 ++++++++++++++ .../common/testExecutionHandler.unit.test.ts | 640 ++++++++++++++++++ .../common/testItemIndex.unit.test.ts | 359 ++++++++++ src/test/vscode-mock.ts | 14 + 7 files changed, 2612 insertions(+) create mode 100644 .github/instructions/testing-workflow.instructions.md create mode 100644 src/test/testing/testController/common/testCoverageHandler.unit.test.ts create mode 100644 src/test/testing/testController/common/testDiscoveryHandler.unit.test.ts create mode 100644 src/test/testing/testController/common/testExecutionHandler.unit.test.ts create mode 100644 src/test/testing/testController/common/testItemIndex.unit.test.ts diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md new file mode 100644 index 000000000000..4478c07b5e6c --- /dev/null +++ b/.github/instructions/testing-workflow.instructions.md @@ -0,0 +1,579 @@ +--- +applyTo: '**/test/**' +--- + +# AI Testing Workflow Guide: Write, Run, and Fix Tests + +This guide provides comprehensive instructions for AI agents on the complete testing workflow: writing tests, running them, diagnosing failures, and fixing issues. Use this guide whenever working with test files or when users request testing tasks. + +## Complete Testing Workflow + +This guide covers the full testing lifecycle: + +1. **📝 Writing Tests** - Create comprehensive test suites +2. **▶️ Running Tests** - Execute tests using VS Code tools +3. **🔍 Diagnosing Issues** - Analyze failures and errors +4. **🛠️ Fixing Problems** - Resolve compilation and runtime issues +5. **✅ Validation** - Ensure coverage and resilience + +### When to Use This Guide + +**User Requests Testing:** + +- "Write tests for this function" +- "Run the tests" +- "Fix the failing tests" +- "Test this code" +- "Add test coverage" + +**File Context Triggers:** + +- Working in `**/test/**` directories +- Files ending in `.test.ts` or `.unit.test.ts` +- Test failures or compilation errors +- Coverage reports or test output analysis + +## Test Types + +When implementing tests as an AI agent, choose between two main types: + +### Unit Tests (`*.unit.test.ts`) + +- **Fast isolated testing** - Mock all external dependencies +- **Use for**: Pure functions, business logic, data transformations +- **Execute with**: `runTests` tool with specific file patterns +- **Mock everything** - VS Code APIs automatically mocked via `/src/test/unittests.ts` + +### Extension Tests (`*.test.ts`) + +- **Full VS Code integration** - Real environment with actual APIs +- **Use for**: Command registration, UI interactions, extension lifecycle +- **Execute with**: VS Code launch configurations or `runTests` tool +- **Slower but comprehensive** - Tests complete user workflows + +## 🤖 Agent Tool Usage for Test Execution + +### Primary Tool: `runTests` + +Use the `runTests` tool to execute tests programmatically rather than terminal commands for better integration and result parsing: + +```typescript +// Run specific test files +await runTests({ + files: ['/absolute/path/to/test.unit.test.ts'], + mode: 'run', +}); + +// Run tests with coverage +await runTests({ + files: ['/absolute/path/to/test.unit.test.ts'], + mode: 'coverage', + coverageFiles: ['/absolute/path/to/source.ts'], +}); + +// Run specific test names +await runTests({ + files: ['/absolute/path/to/test.unit.test.ts'], + testNames: ['should handle edge case', 'should validate input'], +}); +``` + +### Compilation Requirements + +Before running tests, ensure compilation. Always start compilation with `npm run watch-tests` before test execution to ensure TypeScript files are built. Recompile after making import/export changes before running tests, as stubs won't work if they're applied to old compiled JavaScript that doesn't have the updated imports: + +```typescript +// Start watch mode for auto-compilation +await run_in_terminal({ + command: 'npm run watch-tests', + isBackground: true, + explanation: 'Start test compilation in watch mode', +}); + +// Or compile manually +await run_in_terminal({ + command: 'npm run compile-tests', + isBackground: false, + explanation: 'Compile TypeScript test files', +}); +``` + +### Alternative: Terminal Execution + +For targeted test runs when `runTests` tool is unavailable. Note: When a targeted test run yields 0 tests, first verify the compiled JS exists under `out/test` (rootDir is `src`); absence almost always means the test file sits outside `src` or compilation hasn't run yet: + +```typescript +// Run specific test suite +await run_in_terminal({ + command: 'npm run unittest -- --grep "Suite Name"', + isBackground: false, + explanation: 'Run targeted unit tests', +}); +``` + +## 🔍 Diagnosing Test Failures + +### Common Failure Patterns + +**Compilation Errors:** + +```typescript +// Missing imports +if (error.includes('Cannot find module')) { + await addMissingImports(testFile); +} + +// Type mismatches +if (error.includes("Type '" && error.includes("' is not assignable"))) { + await fixTypeIssues(testFile); +} +``` + +**Runtime Errors:** + +```typescript +// Mock setup issues +if (error.includes('stub') || error.includes('mock')) { + await fixMockConfiguration(testFile); +} + +// Assertion failures +if (error.includes('AssertionError')) { + await analyzeAssertionFailure(error); +} +``` + +### Systematic Failure Analysis + +Fix test issues iteratively - run tests, analyze failures, apply fixes, repeat until passing. When unit tests fail with VS Code API errors like `TypeError: X is not a constructor` or `Cannot read properties of undefined (reading 'Y')`, check if VS Code APIs are properly mocked in `/src/test/unittests.ts` - add missing APIs following the existing pattern. + +```typescript +interface TestFailureAnalysis { + type: 'compilation' | 'runtime' | 'assertion' | 'timeout'; + message: string; + location: { file: string; line: number; col: number }; + suggestedFix: string; +} + +function analyzeFailure(failure: TestFailure): TestFailureAnalysis { + if (failure.message.includes('Cannot find module')) { + return { + type: 'compilation', + message: failure.message, + location: failure.location, + suggestedFix: 'Add missing import statement', + }; + } + // ... other failure patterns +} +``` + +### Agent Decision Logic for Test Type Selection + +**Choose Unit Tests (`*.unit.test.ts`) when analyzing:** + +- Functions with clear inputs/outputs and no VS Code API dependencies +- Data transformation, parsing, or utility functions +- Business logic that can be isolated with mocks +- Error handling scenarios with predictable inputs + +**Choose Extension Tests (`*.test.ts`) when analyzing:** + +- Functions that register VS Code commands or use `vscode.*` APIs +- UI components, tree views, or command palette interactions +- File system operations requiring workspace context +- Extension lifecycle events (activation, deactivation) + +**Agent Implementation Pattern:** + +```typescript +function determineTestType(functionCode: string): 'unit' | 'extension' { + if ( + functionCode.includes('vscode.') || + functionCode.includes('commands.register') || + functionCode.includes('window.') || + functionCode.includes('workspace.') + ) { + return 'extension'; + } + return 'unit'; +} +``` + +## 🎯 Step 1: Automated Function Analysis + +As an AI agent, analyze the target function systematically: + +### Code Analysis Checklist + +```typescript +interface FunctionAnalysis { + name: string; + inputs: string[]; // Parameter types and names + outputs: string; // Return type + dependencies: string[]; // External modules/APIs used + sideEffects: string[]; // Logging, file system, network calls + errorPaths: string[]; // Exception scenarios + testType: 'unit' | 'extension'; +} +``` + +### Analysis Implementation + +1. **Read function source** using `read_file` tool +2. **Identify imports** - look for `vscode.*`, `child_process`, `fs`, etc. +3. **Map data flow** - trace inputs through transformations to outputs +4. **Catalog dependencies** - external calls that need mocking +5. **Document side effects** - logging, file operations, state changes + +### Test Setup Differences + +#### Unit Test Setup (\*.unit.test.ts) + +```typescript +// Mock VS Code APIs - handled automatically by unittests.ts +import * as sinon from 'sinon'; +import * as workspaceApis from '../../common/workspace.apis'; // Wrapper functions + +// Stub wrapper functions, not VS Code APIs directly +// Always mock wrapper functions (e.g., workspaceApis.getConfiguration()) instead of +// VS Code APIs directly to avoid stubbing issues +const mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); +``` + +#### Extension Test Setup (\*.test.ts) + +```typescript +// Use real VS Code APIs +import * as vscode from 'vscode'; + +// Real VS Code APIs available - no mocking needed +const config = vscode.workspace.getConfiguration('python'); +``` + +## 🎯 Step 2: Generate Test Coverage Matrix + +Based on function analysis, automatically generate comprehensive test scenarios: + +### Coverage Matrix Generation + +```typescript +interface TestScenario { + category: 'happy-path' | 'edge-case' | 'error-handling' | 'side-effects'; + description: string; + inputs: Record; + expectedOutput?: any; + expectedSideEffects?: string[]; + shouldThrow?: boolean; +} +``` + +### Automated Scenario Creation + +1. **Happy Path**: Normal execution with typical inputs +2. **Edge Cases**: Boundary conditions, empty/null inputs, unusual but valid data +3. **Error Scenarios**: Invalid inputs, dependency failures, exception paths +4. **Side Effects**: Verify logging calls, file operations, state changes + +### Agent Pattern for Scenario Generation + +```typescript +function generateTestScenarios(analysis: FunctionAnalysis): TestScenario[] { + const scenarios: TestScenario[] = []; + + // Generate happy path for each input combination + scenarios.push(...generateHappyPathScenarios(analysis)); + + // Generate edge cases for boundary conditions + scenarios.push(...generateEdgeCaseScenarios(analysis)); + + // Generate error scenarios for each dependency + scenarios.push(...generateErrorScenarios(analysis)); + + return scenarios; +} +``` + +## 🗺️ Step 3: Plan Your Test Coverage + +### Create a Test Coverage Matrix + +#### Main Flows + +- ✅ **Happy path scenarios** - normal expected usage +- ✅ **Alternative paths** - different configuration combinations +- ✅ **Integration scenarios** - multiple features working together + +#### Edge Cases + +- 🔸 **Boundary conditions** - empty inputs, missing data +- 🔸 **Error scenarios** - network failures, permission errors +- 🔸 **Data validation** - invalid inputs, type mismatches + +#### Real-World Scenarios + +- ✅ **Fresh install** - clean slate +- ✅ **Existing user** - migration scenarios +- ✅ **Power user** - complex configurations +- 🔸 **Error recovery** - graceful degradation + +### Example Test Plan Structure + +```markdown +## Test Categories + +### 1. Configuration Migration Tests + +- No legacy settings exist +- Legacy settings already migrated +- Fresh migration needed +- Partial migration required +- Migration failures + +### 2. Configuration Source Tests + +- Global search paths +- Workspace search paths +- Settings precedence +- Configuration errors + +### 3. Path Resolution Tests + +- Absolute vs relative paths +- Workspace folder resolution +- Path validation and filtering + +### 4. Integration Scenarios + +- Combined configurations +- Deduplication logic +- Error handling flows +``` + +## 🔧 Step 4: Set Up Your Test Infrastructure + +### Test File Structure + +```typescript +// 1. Imports - group logically +import assert from 'node:assert'; +import * as sinon from 'sinon'; +import { Uri } from 'vscode'; +import * as logging from '../../../common/logging'; +import * as pathUtils from '../../../common/utils/pathUtils'; +import * as workspaceApis from '../../../common/workspace.apis'; + +// 2. Function under test +import { getAllExtraSearchPaths } from '../../../managers/common/nativePythonFinder'; + +// 3. Mock interfaces +interface MockWorkspaceConfig { + get: sinon.SinonStub; + inspect: sinon.SinonStub; + update: sinon.SinonStub; +} +``` + +### Mock Setup Strategy + +Create minimal mock objects with only required methods and use TypeScript type assertions (e.g., `mockApi as PythonEnvironmentApi`) to satisfy interface requirements instead of implementing all interface methods when only specific methods are needed for the test. Simplify mock setup by only mocking methods actually used in tests and use `as unknown as Type` for TypeScript compatibility. + +```typescript +suite('Function Integration Tests', () => { + // 1. Declare all mocks + let mockGetConfiguration: sinon.SinonStub; + let mockGetWorkspaceFolders: sinon.SinonStub; + let mockTraceLog: sinon.SinonStub; + let mockTraceError: sinon.SinonStub; + let mockTraceWarn: sinon.SinonStub; + + // 2. Mock complex objects + let pythonConfig: MockWorkspaceConfig; + let envConfig: MockWorkspaceConfig; + + setup(() => { + // 3. Initialize all mocks + mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); + mockGetWorkspaceFolders = sinon.stub(workspaceApis, 'getWorkspaceFolders'); + mockTraceLog = sinon.stub(logging, 'traceLog'); + mockTraceError = sinon.stub(logging, 'traceError'); + mockTraceWarn = sinon.stub(logging, 'traceWarn'); + + // 4. Set up default behaviors + mockGetWorkspaceFolders.returns(undefined); + + // 5. Create mock configuration objects + // When fixing mock environment creation, use null to truly omit + // properties rather than undefined + pythonConfig = { + get: sinon.stub(), + inspect: sinon.stub(), + update: sinon.stub(), + }; + + envConfig = { + get: sinon.stub(), + inspect: sinon.stub(), + update: sinon.stub(), + }; + }); + + teardown(() => { + sinon.restore(); // Always clean up! + }); +}); +``` + +## Step 4: Write Tests Using Mock → Run → Assert Pattern + +### The Three-Phase Pattern + +#### Phase 1: Mock (Set up the scenario) + +```typescript +test('Description of what this tests', async () => { + // Mock → Clear description of the scenario + pythonConfig.inspect.withArgs('venvPath').returns({ globalValue: '/path' }); + envConfig.inspect.withArgs('globalSearchPaths').returns({ globalValue: [] }); + mockGetWorkspaceFolders.returns([{ uri: Uri.file('/workspace') }]); +``` + +#### Phase 2: Run (Execute the function) + +```typescript +// Run +const result = await getAllExtraSearchPaths(); +``` + +#### Phase 3: Assert (Verify the behavior) + +```typescript + // Assert - Use set-based comparison for order-agnostic testing + const expected = new Set(['/expected', '/paths']); + const actual = new Set(result); + assert.strictEqual(actual.size, expected.size, 'Should have correct number of paths'); + assert.deepStrictEqual(actual, expected, 'Should contain exactly the expected paths'); + + // Verify side effects + // Use sinon.match() patterns for resilient assertions that don't break on minor output changes + assert(mockTraceLog.calledWith(sinon.match(/completion/i)), 'Should log completion'); +}); +``` + +## Step 6: Make Tests Resilient + +### Use Order-Agnostic Comparisons + +```typescript +// ❌ Brittle - depends on order +assert.deepStrictEqual(result, ['/path1', '/path2', '/path3']); + +// ✅ Resilient - order doesn't matter +const expected = new Set(['/path1', '/path2', '/path3']); +const actual = new Set(result); +assert.strictEqual(actual.size, expected.size, 'Should have correct number of paths'); +assert.deepStrictEqual(actual, expected, 'Should contain exactly the expected paths'); +``` + +### Use Flexible Error Message Testing + +```typescript +// ❌ Brittle - exact text matching +assert(mockTraceError.calledWith('Error during legacy python settings migration:')); + +// ✅ Resilient - pattern matching +assert(mockTraceError.calledWith(sinon.match.string, sinon.match.instanceOf(Error)), 'Should log migration error'); + +// ✅ Resilient - key terms with regex +assert(mockTraceError.calledWith(sinon.match(/migration.*error/i)), 'Should log migration error'); +``` + +### Handle Complex Mock Scenarios + +```typescript +// For functions that call the same mock multiple times +envConfig.inspect.withArgs('globalSearchPaths').returns({ globalValue: [] }); +envConfig.inspect + .withArgs('globalSearchPaths') + .onSecondCall() + .returns({ + globalValue: ['/migrated/paths'], + }); + +// Testing async functions with child processes: +// Call the function first to get a promise, then use setTimeout to emit mock events, +// then await the promise - this ensures proper timing of mock setup versus function execution + +// Cannot stub internal function calls within the same module after import - stub external +// dependencies instead (e.g., stub childProcessApis.spawnProcess rather than trying to stub +// helpers.isUvInstalled when testing helpers.shouldUseUv) because intra-module calls use +// direct references, not module exports +``` + +## 🧪 Step 7: Test Categories and Patterns + +### Configuration Tests + +- Test different setting combinations +- Test setting precedence (workspace > user > default) +- Test configuration errors and recovery +- Always use dynamic path construction with Node.js `path` module when testing functions that resolve paths against workspace folders to ensure cross-platform compatibility + +### Data Flow Tests + +- Test how data moves through the system +- Test transformations (path resolution, filtering) +- Test state changes (migrations, updates) + +### Error Handling Tests + +- Test graceful degradation +- Test error logging +- Test fallback behaviors + +### Integration Tests + +- Test multiple features together +- Test real-world scenarios +- Test edge case combinations + +## 📊 Step 8: Review and Refine + +### Test Quality Checklist + +- [ ] **Clear naming** - test names describe the scenario and expected outcome +- [ ] **Good coverage** - main flows, edge cases, error scenarios +- [ ] **Resilient assertions** - won't break due to minor changes +- [ ] **Readable structure** - follows Mock → Run → Assert pattern +- [ ] **Isolated tests** - each test is independent +- [ ] **Fast execution** - tests run quickly with proper mocking + +### Common Anti-Patterns to Avoid + +- ❌ Testing implementation details instead of behavior +- ❌ Brittle assertions that break on cosmetic changes +- ❌ Order-dependent tests that fail due to processing changes +- ❌ Tests that don't clean up mocks properly +- ❌ Overly complex test setup that's hard to understand + +## 🔄 Reviewing and Improving Existing Tests + +### Quick Review Process + +1. **Read test files** - Check structure and mock setup +2. **Run tests** - Establish baseline functionality +3. **Apply improvements** - Use patterns below. When reviewing existing tests, focus on behavior rather than implementation details in test names and assertions +4. **Verify** - Ensure tests still pass + +### Common Fixes + +- Over-complex mocks → Minimal mocks with only needed methods +- Brittle assertions → Behavior-focused with error messages +- Vague test names → Clear scenario descriptions (transform "should return X when Y" into "should [expected behavior] when [scenario context]") +- Missing structure → Mock → Run → Assert pattern +- Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable) + +## 🧠 Agent Learnings + +- When mocking `testController.createTestItem()` in unit tests, use `typemoq.It.isAny()` for parameters when testing handler behavior (not ID/label generation logic), but consider using specific matchers (e.g., `It.is((id: string) => id.startsWith('_error_'))`) when the actual values being passed are important for correctness - this balances test precision with maintainability (1) +- Remove unused variables from test code immediately - leftover tracking variables like `validationCallCount` that aren't referenced indicate dead code that should be simplified (1) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index a1ed133e11e6..43478179f5b4 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -31,6 +31,7 @@ export class PythonResultResolver implements ITestResultResolver { constructor(testController: TestController, testProvider: TestProvider, private workspaceUri: Uri) { this.testController = testController; this.testProvider = testProvider; + // Initialize a new TestItemIndex which will be used to track test items in this workspace this.testItemIndex = new TestItemIndex(); } diff --git a/src/test/testing/testController/common/testCoverageHandler.unit.test.ts b/src/test/testing/testController/common/testCoverageHandler.unit.test.ts new file mode 100644 index 000000000000..52f32f10dc60 --- /dev/null +++ b/src/test/testing/testController/common/testCoverageHandler.unit.test.ts @@ -0,0 +1,502 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestRun, Uri, FileCoverage } from 'vscode'; +import * as typemoq from 'typemoq'; +import * as assert from 'assert'; +import { TestCoverageHandler } from '../../../../client/testing/testController/common/testCoverageHandler'; +import { CoveragePayload } from '../../../../client/testing/testController/common/types'; + +suite('TestCoverageHandler', () => { + let coverageHandler: TestCoverageHandler; + let runInstanceMock: typemoq.IMock; + + setup(() => { + coverageHandler = new TestCoverageHandler(); + runInstanceMock = typemoq.Mock.ofType(); + }); + + suite('processCoverage', () => { + test('should return empty map for undefined result', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: undefined, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.strictEqual(result.size, 0); + runInstanceMock.verify((r) => r.addCoverage(typemoq.It.isAny()), typemoq.Times.never()); + }); + + test('should create FileCoverage for each file', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file1.py': { + lines_covered: [1, 2, 3], + lines_missed: [4, 5], + executed_branches: 5, + total_branches: 10, + }, + '/path/to/file2.py': { + lines_covered: [1, 2], + lines_missed: [3], + executed_branches: 2, + total_branches: 4, + }, + }, + error: '', + }; + + coverageHandler.processCoverage(payload, runInstanceMock.object); + + runInstanceMock.verify((r) => r.addCoverage(typemoq.It.isAny()), typemoq.Times.exactly(2)); + }); + + test('should call runInstance.addCoverage with correct FileCoverage', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2, 3], + lines_missed: [4, 5], + executed_branches: 5, + total_branches: 10, + }, + }, + error: '', + }; + + let capturedCoverage: FileCoverage | undefined; + runInstanceMock + .setup((r) => r.addCoverage(typemoq.It.isAny())) + .callback((coverage: FileCoverage) => { + capturedCoverage = coverage; + }); + + coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.ok(capturedCoverage); + assert.strictEqual(capturedCoverage!.uri.fsPath, '/path/to/file.py'); + }); + + test('should return detailed coverage map with correct keys', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file1.py': { + lines_covered: [1, 2], + lines_missed: [3], + executed_branches: 2, + total_branches: 4, + }, + '/path/to/file2.py': { + lines_covered: [5, 6, 7], + lines_missed: [], + executed_branches: 3, + total_branches: 3, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.strictEqual(result.size, 2); + assert.ok(result.has(Uri.file('/path/to/file1.py').fsPath)); + assert.ok(result.has(Uri.file('/path/to/file2.py').fsPath)); + }); + + test('should handle empty coverage data', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: {}, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.strictEqual(result.size, 0); + }); + + test('should handle file with no covered lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [], + lines_missed: [1, 2, 3], + executed_branches: 0, + total_branches: 5, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 3); // Only missed lines + }); + + test('should handle file with no missed lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2, 3], + lines_missed: [], + executed_branches: 5, + total_branches: 5, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 3); // Only covered lines + }); + + test('should handle undefined lines_covered', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: undefined as any, + lines_missed: [1, 2], + executed_branches: 0, + total_branches: 2, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 2); // Only missed lines + }); + + test('should handle undefined lines_missed', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2], + lines_missed: undefined as any, + executed_branches: 2, + total_branches: 2, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 2); // Only covered lines + }); + }); + + suite('createFileCoverage', () => { + test('should handle line coverage only when totalBranches is -1', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2, 3], + lines_missed: [4, 5], + executed_branches: 0, + total_branches: -1, // Branch coverage disabled + }, + }, + error: '', + }; + + let capturedCoverage: FileCoverage | undefined; + runInstanceMock + .setup((r) => r.addCoverage(typemoq.It.isAny())) + .callback((coverage: FileCoverage) => { + capturedCoverage = coverage; + }); + + coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.ok(capturedCoverage); + // Branch coverage should not be included + assert.strictEqual((capturedCoverage as any).branchCoverage, undefined); + }); + + test('should include branch coverage when available', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2, 3], + lines_missed: [4], + executed_branches: 7, + total_branches: 10, + }, + }, + error: '', + }; + + let capturedCoverage: FileCoverage | undefined; + runInstanceMock + .setup((r) => r.addCoverage(typemoq.It.isAny())) + .callback((coverage: FileCoverage) => { + capturedCoverage = coverage; + }); + + coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.ok(capturedCoverage); + // Should have branch coverage + assert.ok((capturedCoverage as any).branchCoverage); + }); + + test('should calculate line coverage counts correctly', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2, 3, 4, 5], + lines_missed: [6, 7], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + let capturedCoverage: FileCoverage | undefined; + runInstanceMock + .setup((r) => r.addCoverage(typemoq.It.isAny())) + .callback((coverage: FileCoverage) => { + capturedCoverage = coverage; + }); + + coverageHandler.processCoverage(payload, runInstanceMock.object); + + assert.ok(capturedCoverage); + // 5 covered out of 7 total (5 covered + 2 missed) + assert.strictEqual((capturedCoverage as any).statementCoverage.covered, 5); + assert.strictEqual((capturedCoverage as any).statementCoverage.total, 7); + }); + }); + + suite('createDetailedCoverage', () => { + test('should create StatementCoverage for covered lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 2, 3], + lines_missed: [], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 3); + + // All should be covered (true) + detailedCoverage!.forEach((coverage) => { + assert.strictEqual((coverage as any).executed, true); + }); + }); + + test('should create StatementCoverage for missed lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [], + lines_missed: [1, 2, 3], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 3); + + // All should be NOT covered (false) + detailedCoverage!.forEach((coverage) => { + assert.strictEqual((coverage as any).executed, false); + }); + }); + + test('should convert 1-indexed to 0-indexed line numbers for covered lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 5, 10], + lines_missed: [], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + + // Line 1 should map to range starting at line 0 + assert.strictEqual((detailedCoverage![0] as any).location.start.line, 0); + // Line 5 should map to range starting at line 4 + assert.strictEqual((detailedCoverage![1] as any).location.start.line, 4); + // Line 10 should map to range starting at line 9 + assert.strictEqual((detailedCoverage![2] as any).location.start.line, 9); + }); + + test('should convert 1-indexed to 0-indexed line numbers for missed lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [], + lines_missed: [3, 7, 12], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + + // Line 3 should map to range starting at line 2 + assert.strictEqual((detailedCoverage![0] as any).location.start.line, 2); + // Line 7 should map to range starting at line 6 + assert.strictEqual((detailedCoverage![1] as any).location.start.line, 6); + // Line 12 should map to range starting at line 11 + assert.strictEqual((detailedCoverage![2] as any).location.start.line, 11); + }); + + test('should handle large line numbers', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1000, 5000, 10000], + lines_missed: [], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 3); + + // Verify conversion is correct for large numbers + assert.strictEqual((detailedCoverage![0] as any).location.start.line, 999); + assert.strictEqual((detailedCoverage![1] as any).location.start.line, 4999); + assert.strictEqual((detailedCoverage![2] as any).location.start.line, 9999); + }); + + test('should create detailed coverage with both covered and missed lines', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1, 3, 5], + lines_missed: [2, 4, 6], + executed_branches: 3, + total_branches: 6, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + assert.strictEqual(detailedCoverage!.length, 6); // 3 covered + 3 missed + + // Count covered vs not covered + const covered = detailedCoverage!.filter((c) => (c as any).executed === true); + const notCovered = detailedCoverage!.filter((c) => (c as any).executed === false); + + assert.strictEqual(covered.length, 3); + assert.strictEqual(notCovered.length, 3); + }); + + test('should set range to cover entire line', () => { + const payload: CoveragePayload = { + coverage: true, + cwd: '/foo/bar', + result: { + '/path/to/file.py': { + lines_covered: [1], + lines_missed: [], + executed_branches: 0, + total_branches: -1, + }, + }, + error: '', + }; + + const result = coverageHandler.processCoverage(payload, runInstanceMock.object); + + const detailedCoverage = result.get(Uri.file('/path/to/file.py').fsPath); + assert.ok(detailedCoverage); + + const coverage = detailedCoverage![0] as any; + // Start at column 0 + assert.strictEqual(coverage.location.start.character, 0); + // End at max safe integer (entire line) + assert.strictEqual(coverage.location.end.character, Number.MAX_SAFE_INTEGER); + }); + }); +}); diff --git a/src/test/testing/testController/common/testDiscoveryHandler.unit.test.ts b/src/test/testing/testController/common/testDiscoveryHandler.unit.test.ts new file mode 100644 index 000000000000..458e3d984405 --- /dev/null +++ b/src/test/testing/testController/common/testDiscoveryHandler.unit.test.ts @@ -0,0 +1,517 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestController, TestItem, Uri, CancellationToken, TestItemCollection } from 'vscode'; +import * as typemoq from 'typemoq'; +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { TestDiscoveryHandler } from '../../../../client/testing/testController/common/testDiscoveryHandler'; +import { TestItemIndex } from '../../../../client/testing/testController/common/testItemIndex'; +import { DiscoveredTestPayload, DiscoveredTestNode } from '../../../../client/testing/testController/common/types'; +import { TestProvider } from '../../../../client/testing/types'; +import * as utils from '../../../../client/testing/testController/common/utils'; +import * as testItemUtilities from '../../../../client/testing/testController/common/testItemUtilities'; + +suite('TestDiscoveryHandler', () => { + let discoveryHandler: TestDiscoveryHandler; + let testControllerMock: typemoq.IMock; + let testItemIndexMock: typemoq.IMock; + let testItemCollectionMock: typemoq.IMock; + let workspaceUri: Uri; + let testProvider: TestProvider; + let cancelationToken: CancellationToken; + + setup(() => { + discoveryHandler = new TestDiscoveryHandler(); + testControllerMock = typemoq.Mock.ofType(); + testItemIndexMock = typemoq.Mock.ofType(); + testItemCollectionMock = typemoq.Mock.ofType(); + + // Setup default test controller items mock + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + testItemCollectionMock.setup((x) => x.delete(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + + workspaceUri = Uri.file('/foo/bar'); + testProvider = 'pytest'; + cancelationToken = ({ + isCancellationRequested: false, + } as unknown) as CancellationToken; + }); + + teardown(() => { + sinon.restore(); + }); + + suite('processDiscovery', () => { + test('should handle null payload gracefully', () => { + discoveryHandler.processDiscovery( + null as any, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + // Should not throw and should not call populateTestTree + testItemIndexMock.verify((x) => x.clear(), typemoq.Times.never()); + }); + + test('should call populateTestTree with correct params on success', () => { + const tests: DiscoveredTestNode = { + path: '/foo/bar', + name: 'root', + type_: 'folder', + id_: 'root_id', + children: [], + }; + + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'success', + tests, + }; + + const populateTestTreeStub = sinon.stub(utils, 'populateTestTree'); + testItemIndexMock.setup((x) => x.clear()).returns(() => undefined); + + // Setup map getters for populateTestTree + const mockRunIdMap = new Map(); + const mockVSidMap = new Map(); + const mockVStoRunMap = new Map(); + testItemIndexMock.setup((x) => x.runIdToTestItemMap).returns(() => mockRunIdMap); + testItemIndexMock.setup((x) => x.runIdToVSidMap).returns(() => mockVSidMap); + testItemIndexMock.setup((x) => x.vsIdToRunIdMap).returns(() => mockVStoRunMap); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + testItemIndexMock.verify((x) => x.clear(), typemoq.Times.once()); + assert.ok(populateTestTreeStub.calledOnce); + sinon.assert.calledWith( + populateTestTreeStub, + testControllerMock.object, + tests, + undefined, + sinon.match.any, + cancelationToken, + ); + }); + + test('should clear index before populating', () => { + const tests: DiscoveredTestNode = { + path: '/foo/bar', + name: 'root', + type_: 'folder', + id_: 'root_id', + children: [], + }; + + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'success', + tests, + }; + + sinon.stub(utils, 'populateTestTree'); + + const clearSpy = sinon.spy(); + testItemIndexMock.setup((x) => x.clear()).callback(clearSpy); + testItemIndexMock.setup((x) => x.runIdToTestItemMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.runIdToVSidMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.vsIdToRunIdMap).returns(() => new Map()); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + assert.ok(clearSpy.calledOnce); + }); + + test('should handle error status and create error node', () => { + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'error', + error: ['Error message 1', 'Error message 2'], + }; + + const createErrorNodeSpy = sinon.spy(discoveryHandler, 'createErrorNode'); + + // Mock createTestItem to return a proper TestItem + const mockErrorItem = ({ + id: 'error_id', + error: null, + canResolveChildren: false, + tags: [], + } as unknown) as TestItem; + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => mockErrorItem); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + assert.ok(createErrorNodeSpy.calledOnce); + assert.ok( + createErrorNodeSpy.calledWith(testControllerMock.object, workspaceUri, payload.error, testProvider), + ); + }); + + test('should handle both errors and tests in same payload', () => { + const tests: DiscoveredTestNode = { + path: '/foo/bar', + name: 'root', + type_: 'folder', + id_: 'root_id', + children: [], + }; + + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'error', + error: ['Partial error'], + tests, + }; + + sinon.stub(utils, 'populateTestTree'); + const createErrorNodeSpy = sinon.spy(discoveryHandler, 'createErrorNode'); + + // Mock createTestItem to return a proper TestItem + const mockErrorItem = ({ + id: 'error_id', + error: null, + canResolveChildren: false, + tags: [], + } as unknown) as TestItem; + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => mockErrorItem); + + testItemIndexMock.setup((x) => x.clear()).returns(() => undefined); + testItemIndexMock.setup((x) => x.runIdToTestItemMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.runIdToVSidMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.vsIdToRunIdMap).returns(() => new Map()); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + // Should create error node AND populate test tree + assert.ok(createErrorNodeSpy.calledOnce); + testItemIndexMock.verify((x) => x.clear(), typemoq.Times.once()); + }); + + test('should delete error node on successful discovery', () => { + const tests: DiscoveredTestNode = { + path: '/foo/bar', + name: 'root', + type_: 'folder', + id_: 'root_id', + children: [], + }; + + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'success', + tests, + }; + + const deleteSpy = sinon.spy(); + // Reset and reconfigure the collection mock to capture delete call + testItemCollectionMock.reset(); + testItemCollectionMock + .setup((x) => x.delete(typemoq.It.isAny())) + .callback(deleteSpy) + .returns(() => undefined); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.reset(); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + sinon.stub(utils, 'populateTestTree'); + testItemIndexMock.setup((x) => x.clear()).returns(() => undefined); + testItemIndexMock.setup((x) => x.runIdToTestItemMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.runIdToVSidMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.vsIdToRunIdMap).returns(() => new Map()); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + assert.ok(deleteSpy.calledOnce); + assert.ok(deleteSpy.calledWith(`DiscoveryError:${workspaceUri.fsPath}`)); + }); + + test('should respect cancellation token', () => { + const tests: DiscoveredTestNode = { + path: '/foo/bar', + name: 'root', + type_: 'folder', + id_: 'root_id', + children: [], + }; + + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'success', + tests, + }; + + const populateTestTreeStub = sinon.stub(utils, 'populateTestTree'); + testItemIndexMock.setup((x) => x.clear()).returns(() => undefined); + testItemIndexMock.setup((x) => x.runIdToTestItemMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.runIdToVSidMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.vsIdToRunIdMap).returns(() => new Map()); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + // Verify token was passed to populateTestTree + assert.ok(populateTestTreeStub.calledOnce); + const lastArg = populateTestTreeStub.getCall(0).args[4]; + assert.strictEqual(lastArg, cancelationToken); + }); + + test('should handle null tests in payload', () => { + const payload: DiscoveredTestPayload = { + cwd: workspaceUri.fsPath, + status: 'success', + tests: null as any, + }; + + const populateTestTreeStub = sinon.stub(utils, 'populateTestTree'); + testItemIndexMock.setup((x) => x.clear()).returns(() => undefined); + testItemIndexMock.setup((x) => x.runIdToTestItemMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.runIdToVSidMap).returns(() => new Map()); + testItemIndexMock.setup((x) => x.vsIdToRunIdMap).returns(() => new Map()); + + discoveryHandler.processDiscovery( + payload, + testControllerMock.object, + testItemIndexMock.object, + workspaceUri, + testProvider, + cancelationToken, + ); + + // Should still call populateTestTree with null + assert.ok(populateTestTreeStub.calledOnce); + testItemIndexMock.verify((x) => x.clear(), typemoq.Times.once()); + }); + }); + + suite('createErrorNode', () => { + test('should create error with correct message for pytest', () => { + const error = ['Error line 1', 'Error line 2']; + testProvider = 'pytest'; + + const buildErrorNodeOptionsStub = sinon.stub(utils, 'buildErrorNodeOptions').returns({ + id: 'error_id', + label: 'Error Label', + error: 'Error Message', + }); + + const mockErrorItem = ({ + id: 'error_id', + error: null, + } as unknown) as TestItem; + + const createErrorTestItemStub = sinon.stub(testItemUtilities, 'createErrorTestItem').returns(mockErrorItem); + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + discoveryHandler.createErrorNode(testControllerMock.object, workspaceUri, error, testProvider); + + assert.ok(buildErrorNodeOptionsStub.calledOnce); + assert.ok(createErrorTestItemStub.calledOnce); + assert.ok(mockErrorItem.error !== null); + }); + + test('should create error with correct message for unittest', () => { + const error = ['Unittest error']; + testProvider = 'unittest'; + + sinon.stub(utils, 'buildErrorNodeOptions').returns({ + id: 'error_id', + label: 'Error Label', + error: 'Error Message', + }); + + const mockErrorItem = ({ + id: 'error_id', + error: null, + } as unknown) as TestItem; + + sinon.stub(testItemUtilities, 'createErrorTestItem').returns(mockErrorItem); + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + discoveryHandler.createErrorNode(testControllerMock.object, workspaceUri, error, testProvider); + + assert.ok(mockErrorItem.error !== null); + }); + + test('should set markdown error label correctly', () => { + const error = ['Test error']; + + sinon.stub(utils, 'buildErrorNodeOptions').returns({ + id: 'error_id', + label: 'Error Label', + error: 'Error Message', + }); + + const mockErrorItem = ({ + id: 'error_id', + error: null, + } as unknown) as TestItem; + + sinon.stub(testItemUtilities, 'createErrorTestItem').returns(mockErrorItem); + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + discoveryHandler.createErrorNode(testControllerMock.object, workspaceUri, error, testProvider); + + assert.ok(mockErrorItem.error); + assert.strictEqual( + (mockErrorItem.error as any).value, + '[Show output](command:python.viewOutput) to view error logs', + ); + assert.strictEqual((mockErrorItem.error as any).isTrusted, true); + }); + + test('should handle undefined error array', () => { + sinon.stub(utils, 'buildErrorNodeOptions').returns({ + id: 'error_id', + label: 'Error Label', + error: 'Error Message', + }); + + const mockErrorItem = ({ + id: 'error_id', + error: null, + } as unknown) as TestItem; + + sinon.stub(testItemUtilities, 'createErrorTestItem').returns(mockErrorItem); + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + discoveryHandler.createErrorNode(testControllerMock.object, workspaceUri, undefined, testProvider); + + // Should not throw + assert.ok(mockErrorItem.error !== null); + }); + + test('should reuse existing error node if present', () => { + const error = ['Error']; + + // Create a proper object with settable error property + const existingErrorItem: any = { + id: `DiscoveryError:${workspaceUri.fsPath}`, + error: null, + canResolveChildren: false, + tags: [], + }; + + sinon.stub(utils, 'buildErrorNodeOptions').returns({ + id: `DiscoveryError:${workspaceUri.fsPath}`, + label: 'Error Label', + error: 'Error Message', + }); + + const createErrorTestItemStub = sinon.stub(testItemUtilities, 'createErrorTestItem'); + + // Reset and setup collection to return existing item + testItemCollectionMock.reset(); + testItemCollectionMock + .setup((x) => x.get(`DiscoveryError:${workspaceUri.fsPath}`)) + .returns(() => existingErrorItem); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.reset(); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + discoveryHandler.createErrorNode(testControllerMock.object, workspaceUri, error, testProvider); + + // Should not create a new error item + assert.ok(createErrorTestItemStub.notCalled); + // Should still update the error property + assert.ok(existingErrorItem.error !== null); + }); + + test('should handle multiple error messages', () => { + const error = ['Error 1', 'Error 2', 'Error 3']; + + const buildStub = sinon.stub(utils, 'buildErrorNodeOptions').returns({ + id: 'error_id', + label: 'Error Label', + error: 'Error Message', + }); + + const mockErrorItem = ({ + id: 'error_id', + error: null, + } as unknown) as TestItem; + + sinon.stub(testItemUtilities, 'createErrorTestItem').returns(mockErrorItem); + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testItemCollectionMock.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + discoveryHandler.createErrorNode(testControllerMock.object, workspaceUri, error, testProvider); + + // Verify the error messages are joined + const expectedMessage = sinon.match((value: string) => { + return value.includes('Error 1') && value.includes('Error 2') && value.includes('Error 3'); + }); + sinon.assert.calledWith(buildStub, workspaceUri, expectedMessage, testProvider); + }); + }); +}); diff --git a/src/test/testing/testController/common/testExecutionHandler.unit.test.ts b/src/test/testing/testController/common/testExecutionHandler.unit.test.ts new file mode 100644 index 000000000000..e0935d358dde --- /dev/null +++ b/src/test/testing/testController/common/testExecutionHandler.unit.test.ts @@ -0,0 +1,640 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestController, TestItem, TestRun, TestMessage, Uri, Range, TestItemCollection, MarkdownString } from 'vscode'; +import * as typemoq from 'typemoq'; +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { TestExecutionHandler } from '../../../../client/testing/testController/common/testExecutionHandler'; +import { TestItemIndex } from '../../../../client/testing/testController/common/testItemIndex'; +import { ExecutionTestPayload } from '../../../../client/testing/testController/common/types'; + +suite('TestExecutionHandler', () => { + let executionHandler: TestExecutionHandler; + let testControllerMock: typemoq.IMock; + let testItemIndexMock: typemoq.IMock; + let runInstanceMock: typemoq.IMock; + let mockTestItem: TestItem; + let mockParentItem: TestItem; + + setup(() => { + executionHandler = new TestExecutionHandler(); + testControllerMock = typemoq.Mock.ofType(); + testItemIndexMock = typemoq.Mock.ofType(); + runInstanceMock = typemoq.Mock.ofType(); + + mockTestItem = createMockTestItem('test1', 'Test 1'); + mockParentItem = createMockTestItem('parentTest', 'Parent Test'); + }); + + teardown(() => { + sinon.restore(); + }); + + suite('processExecution', () => { + test('should return empty stats for empty payload', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: {}, + error: '', + }; + + const stats = executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.strictEqual(stats.size, 0); + }); + + test('should return empty stats for undefined result', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + error: '', + }; + + const stats = executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.strictEqual(stats.size, 0); + }); + + test('should process multiple test results', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { test: 'test1', outcome: 'success', message: '', traceback: '' }, + test2: { test: 'test2', outcome: 'failure', message: 'Failed', traceback: 'traceback' }, + }, + error: '', + }; + + const mockTestItem2 = createMockTestItem('test2', 'Test 2'); + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + testItemIndexMock + .setup((x) => x.getTestItem('test2', testControllerMock.object)) + .returns(() => mockTestItem2); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.passed(mockTestItem), typemoq.Times.once()); + runInstanceMock.verify((r) => r.failed(mockTestItem2, typemoq.It.isAny()), typemoq.Times.once()); + }); + }); + + suite('handleTestError', () => { + test('should create error message with traceback', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'error', + message: 'Error occurred', + traceback: 'line1\nline2\nline3', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + let capturedMessage: TestMessage | undefined; + runInstanceMock + .setup((r) => r.errored(mockTestItem, typemoq.It.isAny())) + .callback((_, message: TestMessage) => { + capturedMessage = message; + }); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.ok(capturedMessage); + const messageText = + capturedMessage!.message instanceof MarkdownString + ? capturedMessage!.message.value + : capturedMessage!.message; + assert.ok(messageText.includes('Error occurred')); + assert.ok(messageText.includes('line1')); + assert.ok(messageText.includes('line2')); + runInstanceMock.verify((r) => r.errored(mockTestItem, typemoq.It.isAny()), typemoq.Times.once()); + }); + + test('should set location when test item has range', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'error', + message: 'Error', + traceback: '', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + let capturedMessage: TestMessage | undefined; + runInstanceMock + .setup((r) => r.errored(mockTestItem, typemoq.It.isAny())) + .callback((_, message: TestMessage) => { + capturedMessage = message; + }); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.ok(capturedMessage); + assert.ok(capturedMessage!.location); + assert.strictEqual(capturedMessage!.location!.uri.fsPath, mockTestItem.uri!.fsPath); + }); + + test('should handle missing traceback', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'error', + message: 'Error', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.errored(mockTestItem, typemoq.It.isAny()), typemoq.Times.once()); + }); + }); + + suite('handleTestFailure', () => { + test('should create failure message with traceback', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'failure', + message: 'Assertion failed', + traceback: 'AssertionError\nline1', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + let capturedMessage: TestMessage | undefined; + runInstanceMock + .setup((r) => r.failed(mockTestItem, typemoq.It.isAny())) + .callback((_, message: TestMessage) => { + capturedMessage = message; + }); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.ok(capturedMessage); + const messageText = + capturedMessage!.message instanceof MarkdownString + ? capturedMessage!.message.value + : capturedMessage!.message; + assert.ok(messageText.includes('Assertion failed')); + assert.ok(messageText.includes('AssertionError')); + runInstanceMock.verify((r) => r.failed(mockTestItem, typemoq.It.isAny()), typemoq.Times.once()); + }); + + test('should handle passed-unexpected outcome', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'passed-unexpected', + message: 'Unexpected pass', + traceback: '', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.failed(mockTestItem, typemoq.It.isAny()), typemoq.Times.once()); + }); + }); + + suite('handleTestSuccess', () => { + test('should mark test as passed', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'success', + message: '', + traceback: '', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.passed(mockTestItem), typemoq.Times.once()); + }); + + test('should handle expected-failure outcome', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'expected-failure', + message: '', + traceback: '', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.passed(mockTestItem), typemoq.Times.once()); + }); + + test('should not call passed when test item not found', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'success', + message: '', + traceback: '', + }, + }, + error: '', + }; + + testItemIndexMock.setup((x) => x.getTestItem('test1', testControllerMock.object)).returns(() => undefined); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.passed(typemoq.It.isAny()), typemoq.Times.never()); + }); + }); + + suite('handleTestSkipped', () => { + test('should mark test as skipped', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + test1: { + test: 'test1', + outcome: 'skipped', + message: 'Test skipped', + traceback: '', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('test1', testControllerMock.object)) + .returns(() => mockTestItem); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.skipped(mockTestItem), typemoq.Times.once()); + }); + }); + + suite('handleSubtestFailure', () => { + test('should create child test item for subtest', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest1)': { + test: 'parentTest', + outcome: 'subtest-failure', + message: 'Subtest failed', + traceback: 'traceback', + subtest: 'subtest1', + }, + }, + error: '', + }; + + const mockSubtestItem = createMockTestItem('subtest1', 'Subtest 1'); + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => mockSubtestItem); + + const stats = executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.strictEqual(stats.size, 1); + assert.strictEqual(stats.get('parentTest')?.failed, 1); + assert.strictEqual(stats.get('parentTest')?.passed, 0); + + runInstanceMock.verify((r) => r.started(mockSubtestItem), typemoq.Times.once()); + runInstanceMock.verify((r) => r.failed(mockSubtestItem, typemoq.It.isAny()), typemoq.Times.once()); + }); + + test('should update stats correctly for multiple subtests', () => { + const payload1: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest1)': { + test: 'parentTest', + outcome: 'subtest-failure', + message: 'Failed', + traceback: '', + subtest: 'subtest1', + }, + }, + error: '', + }; + + const payload2: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest2)': { + test: 'parentTest', + outcome: 'subtest-failure', + message: 'Failed', + traceback: '', + subtest: 'subtest2', + }, + }, + error: '', + }; + + const mockSubtest1 = createMockTestItem('subtest1', 'Subtest 1'); + const mockSubtest2 = createMockTestItem('subtest2', 'Subtest 2'); + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + + // Return different items based on call order + let callCount = 0; + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => { + callCount++; + return callCount === 1 ? mockSubtest1 : mockSubtest2; + }); + + const stats1 = executionHandler.processExecution( + payload1, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Process second subtest with stats from first + const stats2 = executionHandler.processExecution( + payload2, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Verify stats are separate for each call + assert.strictEqual(stats1.get('parentTest')?.failed, 1); + assert.strictEqual(stats2.get('parentTest')?.failed, 1); + }); + + test('should throw error when parent test item not found', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest1)': { + test: 'parentTest', + outcome: 'subtest-failure', + message: 'Failed', + traceback: '', + subtest: 'subtest1', + }, + }, + error: '', + }; + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => undefined); + + assert.throws(() => { + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + }, /Parent test item not found/); + }); + }); + + suite('handleSubtestSuccess', () => { + test('should create passing subtest', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest1)': { + test: 'parentTest', + outcome: 'subtest-success', + message: '', + traceback: '', + subtest: 'subtest1', + }, + }, + error: '', + }; + + const mockSubtestItem = createMockTestItem('subtest1', 'Subtest 1'); + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => mockSubtestItem); + + const stats = executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + assert.strictEqual(stats.size, 1); + assert.strictEqual(stats.get('parentTest')?.passed, 1); + assert.strictEqual(stats.get('parentTest')?.failed, 0); + + runInstanceMock.verify((r) => r.started(mockSubtestItem), typemoq.Times.once()); + runInstanceMock.verify((r) => r.passed(mockSubtestItem), typemoq.Times.once()); + }); + + test('should handle subtest with special characters in name', () => { + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest [subtest with spaces and [brackets]]': { + test: 'parentTest', + outcome: 'subtest-success', + message: '', + traceback: '', + subtest: 'subtest with spaces and [brackets]', + }, + }, + error: '', + }; + + const mockSubtestItem = createMockTestItem('[subtest with spaces and [brackets]]', 'Subtest'); + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => mockSubtestItem); + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + runInstanceMock.verify((r) => r.passed(mockSubtestItem), typemoq.Times.once()); + }); + }); +}); + +function createMockTestItem(id: string, label: string): TestItem { + const range = new Range(0, 0, 0, 0); + const mockChildren = typemoq.Mock.ofType(); + mockChildren.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + + const mockTestItem = ({ + id, + label, + canResolveChildren: false, + tags: [], + children: mockChildren.object, + range, + uri: Uri.file('/foo/bar/test.py'), + parent: undefined, + } as unknown) as TestItem; + + return mockTestItem; +} diff --git a/src/test/testing/testController/common/testItemIndex.unit.test.ts b/src/test/testing/testController/common/testItemIndex.unit.test.ts new file mode 100644 index 000000000000..6712d90ff667 --- /dev/null +++ b/src/test/testing/testController/common/testItemIndex.unit.test.ts @@ -0,0 +1,359 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { TestController, TestItem, Uri, Range, TestItemCollection } from 'vscode'; +import * as typemoq from 'typemoq'; +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { TestItemIndex } from '../../../../client/testing/testController/common/testItemIndex'; + +suite('TestItemIndex', () => { + let testItemIndex: TestItemIndex; + let testControllerMock: typemoq.IMock; + let mockTestItem1: TestItem; + let mockTestItem2: TestItem; + let mockParentItem: TestItem; + + setup(() => { + testItemIndex = new TestItemIndex(); + testControllerMock = typemoq.Mock.ofType(); + + // Create mock test items + mockTestItem1 = createMockTestItem('test1', 'Test 1'); + mockTestItem2 = createMockTestItem('test2', 'Test 2'); + mockParentItem = createMockTestItem('parent', 'Parent'); + }); + + teardown(() => { + sinon.restore(); + }); + + suite('registerTestItem', () => { + test('should store all three mappings correctly', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'test_file.py::test_example'; + + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId), mockTestItem1); + assert.strictEqual(testItemIndex.runIdToVSidMap.get(runId), vsId); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.get(vsId), runId); + }); + + test('should overwrite existing mappings', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'test_file.py::test_example'; + + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + testItemIndex.registerTestItem(runId, vsId, mockTestItem2); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId), mockTestItem2); + }); + + test('should handle different runId and vsId', () => { + const runId = 'test_file.py::TestClass::test_method'; + const vsId = 'different_id'; + + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + assert.strictEqual(testItemIndex.runIdToVSidMap.get(runId), vsId); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.get(vsId), runId); + }); + }); + + suite('getTestItem', () => { + test('should return item on direct lookup when valid', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'test_file.py::test_example'; + + // Register the item + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + // Mock the validation to return true + const isValidStub = sinon.stub(testItemIndex, 'isTestItemValid').returns(true); + + const result = testItemIndex.getTestItem(runId, testControllerMock.object); + + assert.strictEqual(result, mockTestItem1); + assert.ok(isValidStub.calledOnce); + }); + + test('should remove stale item and try vsId fallback', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'test_file.py::test_example'; + + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + // Mock validation to fail on first call (stale item) + const isValidStub = sinon.stub(testItemIndex, 'isTestItemValid').returns(false); + + // Setup controller to not find the item + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.forEach(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.getTestItem(runId, testControllerMock.object); + + // Should have removed the stale item + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId), undefined); + assert.strictEqual(result, undefined); + assert.ok(isValidStub.calledOnce); + }); + + test('should perform vsId search when direct lookup is stale', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'test_file.py::test_example'; + + // Create test item with correct ID + const searchableTestItem = createMockTestItem(vsId, 'Test Example'); + + testItemIndex.registerTestItem(runId, vsId, searchableTestItem); + + // First validation fails (stale), need to search by vsId + sinon.stub(testItemIndex, 'isTestItemValid').returns(false); + + // Setup controller to find item by vsId + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock + .setup((x) => x.forEach(typemoq.It.isAny())) + .callback((callback) => { + callback(searchableTestItem); + }) + .returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.getTestItem(runId, testControllerMock.object); + + // Should recache the found item + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId), searchableTestItem); + assert.strictEqual(result, searchableTestItem); + }); + + test('should return undefined if not found anywhere', () => { + const runId = 'nonexistent'; + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.forEach(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.getTestItem(runId, testControllerMock.object); + + assert.strictEqual(result, undefined); + }); + }); + + suite('getRunId and getVSId', () => { + test('getRunId should convert VS Code ID to Python run ID', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'vscode_id'; + + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + assert.strictEqual(testItemIndex.getRunId(vsId), runId); + }); + + test('getRunId should return undefined for unknown vsId', () => { + assert.strictEqual(testItemIndex.getRunId('unknown'), undefined); + }); + + test('getVSId should convert Python run ID to VS Code ID', () => { + const runId = 'test_file.py::test_example'; + const vsId = 'vscode_id'; + + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + assert.strictEqual(testItemIndex.getVSId(runId), vsId); + }); + + test('getVSId should return undefined for unknown runId', () => { + assert.strictEqual(testItemIndex.getVSId('unknown'), undefined); + }); + }); + + suite('clear', () => { + test('should remove all mappings', () => { + testItemIndex.registerTestItem('runId1', 'vsId1', mockTestItem1); + testItemIndex.registerTestItem('runId2', 'vsId2', mockTestItem2); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.size, 2); + assert.strictEqual(testItemIndex.runIdToVSidMap.size, 2); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.size, 2); + + testItemIndex.clear(); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.size, 0); + assert.strictEqual(testItemIndex.runIdToVSidMap.size, 0); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.size, 0); + }); + + test('should handle clearing empty index', () => { + testItemIndex.clear(); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.size, 0); + assert.strictEqual(testItemIndex.runIdToVSidMap.size, 0); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.size, 0); + }); + }); + + suite('isTestItemValid', () => { + test('should return true for item with valid parent chain leading to controller', () => { + const childItem = createMockTestItem('child', 'Child'); + (childItem as any).parent = mockParentItem; + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(mockParentItem.id)).returns(() => mockParentItem); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.isTestItemValid(childItem, testControllerMock.object); + + assert.strictEqual(result, true); + }); + + test('should return false for orphaned item', () => { + const orphanedItem = createMockTestItem('orphaned', 'Orphaned'); + (orphanedItem as any).parent = mockParentItem; + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.isTestItemValid(orphanedItem, testControllerMock.object); + + assert.strictEqual(result, false); + }); + + test('should return true for root item in controller', () => { + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(mockTestItem1.id)).returns(() => mockTestItem1); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.isTestItemValid(mockTestItem1, testControllerMock.object); + + assert.strictEqual(result, true); + }); + + test('should return false for item not in controller and no parent', () => { + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock.setup((x) => x.get(typemoq.It.isAny())).returns(() => undefined); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + + const result = testItemIndex.isTestItemValid(mockTestItem1, testControllerMock.object); + + assert.strictEqual(result, false); + }); + }); + + suite('cleanupStaleReferences', () => { + test('should remove items not in controller', () => { + const runId1 = 'test1'; + const runId2 = 'test2'; + const vsId1 = 'vs1'; + const vsId2 = 'vs2'; + + testItemIndex.registerTestItem(runId1, vsId1, mockTestItem1); + testItemIndex.registerTestItem(runId2, vsId2, mockTestItem2); + + // Mock validation: first item invalid, second valid + const isValidStub = sinon.stub(testItemIndex, 'isTestItemValid'); + isValidStub.onFirstCall().returns(false); // mockTestItem1 is invalid + isValidStub.onSecondCall().returns(true); // mockTestItem2 is valid + + testItemIndex.cleanupStaleReferences(testControllerMock.object); + + // First item should be removed + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId1), undefined); + assert.strictEqual(testItemIndex.runIdToVSidMap.get(runId1), undefined); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.get(vsId1), undefined); + + // Second item should remain + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId2), mockTestItem2); + assert.strictEqual(testItemIndex.runIdToVSidMap.get(runId2), vsId2); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.get(vsId2), runId2); + }); + + test('should keep all valid items', () => { + const runId1 = 'test1'; + const vsId1 = 'vs1'; + + testItemIndex.registerTestItem(runId1, vsId1, mockTestItem1); + + sinon.stub(testItemIndex, 'isTestItemValid').returns(true); + + testItemIndex.cleanupStaleReferences(testControllerMock.object); + + // Item should still be there + assert.strictEqual(testItemIndex.runIdToTestItemMap.get(runId1), mockTestItem1); + assert.strictEqual(testItemIndex.runIdToVSidMap.get(runId1), vsId1); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.get(vsId1), runId1); + }); + + test('should handle empty index', () => { + testItemIndex.cleanupStaleReferences(testControllerMock.object); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.size, 0); + }); + + test('should remove all items when all are invalid', () => { + testItemIndex.registerTestItem('test1', 'vs1', mockTestItem1); + testItemIndex.registerTestItem('test2', 'vs2', mockTestItem2); + + sinon.stub(testItemIndex, 'isTestItemValid').returns(false); + + testItemIndex.cleanupStaleReferences(testControllerMock.object); + + assert.strictEqual(testItemIndex.runIdToTestItemMap.size, 0); + assert.strictEqual(testItemIndex.runIdToVSidMap.size, 0); + assert.strictEqual(testItemIndex.vsIdToRunIdMap.size, 0); + }); + }); + + suite('Backward compatibility getters', () => { + test('runIdToTestItemMap should return the internal map', () => { + const runId = 'test1'; + testItemIndex.registerTestItem(runId, 'vs1', mockTestItem1); + + const map = testItemIndex.runIdToTestItemMap; + + assert.strictEqual(map.get(runId), mockTestItem1); + }); + + test('runIdToVSidMap should return the internal map', () => { + const runId = 'test1'; + const vsId = 'vs1'; + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + const map = testItemIndex.runIdToVSidMap; + + assert.strictEqual(map.get(runId), vsId); + }); + + test('vsIdToRunIdMap should return the internal map', () => { + const runId = 'test1'; + const vsId = 'vs1'; + testItemIndex.registerTestItem(runId, vsId, mockTestItem1); + + const map = testItemIndex.vsIdToRunIdMap; + + assert.strictEqual(map.get(vsId), runId); + }); + }); +}); + +function createMockTestItem(id: string, label: string): TestItem { + const range = new Range(0, 0, 0, 0); + const mockChildren = typemoq.Mock.ofType(); + mockChildren.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + + const mockTestItem = ({ + id, + label, + canResolveChildren: false, + tags: [], + children: mockChildren.object, + range, + uri: Uri.file('/foo/bar'), + parent: undefined, + } as unknown) as TestItem; + + return mockTestItem; +} diff --git a/src/test/vscode-mock.ts b/src/test/vscode-mock.ts index 0605b1718166..3e2816afbbde 100644 --- a/src/test/vscode-mock.ts +++ b/src/test/vscode-mock.ts @@ -134,3 +134,17 @@ mockedVSCode.LogLevel = vscodeMocks.LogLevel; (mockedVSCode as any).CancellationError = vscodeMocks.vscMockExtHostedTypes.CancellationError; (mockedVSCode as any).LSPCancellationError = vscodeMocks.vscMockExtHostedTypes.LSPCancellationError; mockedVSCode.TestRunProfileKind = vscodeMocks.TestRunProfileKind; +(mockedVSCode as any).TestCoverageCount = class TestCoverageCount { + constructor(public covered: number, public total: number) {} +}; +(mockedVSCode as any).FileCoverage = class FileCoverage { + constructor( + public uri: any, + public statementCoverage: any, + public branchCoverage?: any, + public declarationCoverage?: any, + ) {} +}; +(mockedVSCode as any).StatementCoverage = class StatementCoverage { + constructor(public executed: number | boolean, public location: any, public branches?: any) {} +}; From 773ccb1eb7f2c75997677c5606316d826a42ef04 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 30 Nov 2025 20:24:50 -0800 Subject: [PATCH 04/10] fix failing tests --- .../resultResolver.unit.test.ts | 49 ++++++++++++++++--- 1 file changed, 41 insertions(+), 8 deletions(-) diff --git a/src/test/testing/testController/resultResolver.unit.test.ts b/src/test/testing/testController/resultResolver.unit.test.ts index 05d2ee1dd0f3..e4b350a20750 100644 --- a/src/test/testing/testController/resultResolver.unit.test.ts +++ b/src/test/testing/testController/resultResolver.unit.test.ts @@ -93,12 +93,13 @@ suite('Result Resolver tests', () => { // assert the stub functions were called with the correct parameters // header of populateTestTree is (testController: TestController, testTreeData: DiscoveredTestNode, testRoot: TestItem | undefined, resultResolver: ITestResultResolver, token?: CancellationToken) + // After refactor, an inline object with testItemIndex maps is passed instead of resultResolver sinon.assert.calledWithMatch( populateTestTreeStub, testController, // testController tests, // testTreeData undefined, // testRoot - resultResolver, // resultResolver + sinon.match.has('runIdToTestItem'), // inline object with maps cancelationToken, // token ); }); @@ -182,12 +183,13 @@ suite('Result Resolver tests', () => { sinon.assert.calledWithMatch(createErrorTestItemStub, sinon.match.any, sinon.match.any); // also calls populateTestTree with the discovery test results + // After refactor, an inline object with testItemIndex maps is passed instead of resultResolver sinon.assert.calledWithMatch( populateTestTreeStub, testController, // testController tests, // testTreeData undefined, // testRoot - resultResolver, // resultResolver + sinon.match.has('runIdToTestItem'), // inline object with maps cancelationToken, // token ); }); @@ -327,6 +329,34 @@ suite('Result Resolver tests', () => { sinon.stub(testItemUtilities, 'clearAllChildren').callsFake(() => undefined); testProvider = 'unittest'; workspaceUri = Uri.file('/foo/bar'); + + // Create parent test item with correct ID + const mockParentItem = createMockTestItem('parentTest'); + + // Update testControllerMock to include parent item in its collection + const mockTestItems: [string, TestItem][] = [ + ['1', mockTestItem1], + ['2', mockTestItem2], + ['parentTest', mockParentItem], + ]; + const iterableMock = mockTestItems[Symbol.iterator](); + + const testItemCollectionMock = typemoq.Mock.ofType(); + testItemCollectionMock + .setup((x) => x.forEach(typemoq.It.isAny())) + .callback((callback) => { + let result = iterableMock.next(); + while (!result.done) { + callback(result.value[1]); + result = iterableMock.next(); + } + }) + .returns(() => mockTestItem1); + testItemCollectionMock.setup((x) => x.get('parentTest')).returns(() => mockParentItem); + + testControllerMock.reset(); + testControllerMock.setup((t) => t.items).returns(() => testItemCollectionMock.object); + resultResolver = new ResultResolver.PythonResultResolver( testControllerMock.object, testProvider, @@ -334,13 +364,16 @@ suite('Result Resolver tests', () => { ); const subtestName = 'parentTest [subTest with spaces and [brackets]]'; const mockSubtestItem = createMockTestItem(subtestName); + // add a mock test item to the map of known VSCode ids to run ids resultResolver.runIdToVSid.set('mockTestItem2', 'mockTestItem2'); // creates a mock test item with a space which will be used to split the runId resultResolver.runIdToVSid.set(subtestName, subtestName); + // Register parent test in testItemIndex so it can be found by getTestItem + resultResolver.runIdToVSid.set('parentTest', 'parentTest'); // add this mock test to the map of known test items - resultResolver.runIdToTestItem.set('parentTest', mockTestItem2); + resultResolver.runIdToTestItem.set('parentTest', mockParentItem); resultResolver.runIdToTestItem.set(subtestName, mockSubtestItem); let generatedId: string | undefined; @@ -563,15 +596,15 @@ suite('Result Resolver tests', () => { function createMockTestItem(id: string): TestItem { const range = new Range(0, 0, 0, 0); + const mockChildren = typemoq.Mock.ofType(); + mockChildren.setup((x) => x.add(typemoq.It.isAny())).returns(() => undefined); + mockChildren.setup((x) => x.forEach(typemoq.It.isAny())).returns(() => undefined); + const mockTestItem = ({ id, canResolveChildren: false, tags: [], - children: { - add: () => { - // empty - }, - }, + children: mockChildren.object, range, uri: Uri.file('/foo/bar'), } as unknown) as TestItem; From d6cdb0696c5d56d812ca953581da2b046f80be44 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 30 Nov 2025 20:46:28 -0800 Subject: [PATCH 05/10] fix tests --- .../testing/common/testingAdapter.test.ts | 45 ++++++++++--------- .../common/testCoverageHandler.unit.test.ts | 2 +- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index 97c04d5dfdf1..83e57827f96b 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -161,7 +161,7 @@ suite('End to End Tests: test adapters', () => { resultResolver = new PythonResultResolver(testController, unittestProvider, workspaceUri); let callCount = 0; // const deferredTillEOT = createTestingDeferred(); - resultResolver._resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; @@ -202,7 +202,7 @@ suite('End to End Tests: test adapters', () => { }; resultResolver = new PythonResultResolver(testController, unittestProvider, workspaceUri); let callCount = 0; - resultResolver._resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; @@ -242,7 +242,7 @@ suite('End to End Tests: test adapters', () => { workspaceUri = Uri.parse(rootPathSmallWorkspace); resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver._resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = async (payload, _token?) => { callCount = callCount + 1; actualData = payload; return Promise.resolve(); @@ -291,7 +291,7 @@ suite('End to End Tests: test adapters', () => { resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver._resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; @@ -375,7 +375,7 @@ suite('End to End Tests: test adapters', () => { resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver._resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; @@ -446,7 +446,7 @@ suite('End to End Tests: test adapters', () => { }; resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver._resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; @@ -480,7 +480,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. @@ -554,7 +554,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. @@ -625,7 +625,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. @@ -811,7 +811,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = async (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. @@ -878,7 +878,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveDiscovery = async (data, _token?) => { + resultResolver.resolveDiscovery = async (data, _token?) => { // do the following asserts for each time resolveExecution is called, should be called once per test. callCount = callCount + 1; traceLog(`unittest discovery adapter seg fault error handling \n ${JSON.stringify(data)}`); @@ -931,7 +931,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveDiscovery = async (data, _token?) => { + resultResolver.resolveDiscovery = async (data, _token?) => { // do the following asserts for each time resolveExecution is called, should be called once per test. callCount = callCount + 1; traceLog(`add one to call count, is now ${callCount}`); @@ -984,7 +984,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver._resolveExecution = async (data, _token?) => { + resultResolver.resolveExecution = async (data, _token?) => { // do the following asserts for each time resolveExecution is called, should be called once per test. console.log(`pytest execution adapter seg fault error handling \n ${JSON.stringify(data)}`); callCount = callCount + 1; @@ -1038,8 +1038,8 @@ suite('End to End Tests: test adapters', () => { }); }); - test('_resolveExecution performance test: validates efficient test result processing', async () => { - // This test validates that _resolveExecution processes test results efficiently + 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) @@ -1085,9 +1085,13 @@ suite('End to End Tests: test adapters', () => { const testItemUtilities = require('../../../client/testing/testController/common/testItemUtilities'); testItemUtilities.getTestCaseNodes = getTestCaseNodesSpy; + // Stub isTestItemValid to always return true for performance test + // This prevents expensive tree searches during validation + const testItemIndexStub = sinon.stub((resultResolver as any).testItemIndex, 'isTestItemValid').returns(true); + // Wrap the _resolveExecution function to measure performance - const original_resolveExecution = resultResolver._resolveExecution.bind(resultResolver); - resultResolver._resolveExecution = async (payload, runInstance) => { + const original_resolveExecution = resultResolver.resolveExecution.bind(resultResolver); + resultResolver.resolveExecution = async (payload, runInstance) => { const startTime = performance.now(); callCount++; @@ -1189,8 +1193,8 @@ suite('End to End Tests: test adapters', () => { const overallStartTime = performance.now(); - // Run the _resolveExecution function with test data - await resultResolver._resolveExecution(payload, mockRunInstance as any); + // Run the resolveExecution function with test data + await resultResolver.resolveExecution(payload, mockRunInstance as any); const overallEndTime = performance.now(); const totalTime = overallEndTime - overallStartTime; @@ -1199,6 +1203,7 @@ suite('End to End Tests: test adapters', () => { // CLEANUP: Restore original functions // ================================================================ testItemUtilities.getTestCaseNodes = originalGetTestCaseNodes; + testItemIndexStub.restore(); // ================================================================ // ASSERT: Verify efficient performance characteristics @@ -1214,7 +1219,7 @@ suite('End to End Tests: test adapters', () => { console.log(`Results processed: ${numParameterizedResults}`); // Basic function call verification - assert.strictEqual(callCount, 1, 'Expected _resolveExecution to be called once'); + assert.strictEqual(callCount, 1, 'Expected resolveExecution to be called once'); // EFFICIENCY VERIFICATION: Ensure minimal expensive operations assert.strictEqual( diff --git a/src/test/testing/testController/common/testCoverageHandler.unit.test.ts b/src/test/testing/testController/common/testCoverageHandler.unit.test.ts index 52f32f10dc60..a81aed591128 100644 --- a/src/test/testing/testController/common/testCoverageHandler.unit.test.ts +++ b/src/test/testing/testController/common/testCoverageHandler.unit.test.ts @@ -82,7 +82,7 @@ suite('TestCoverageHandler', () => { coverageHandler.processCoverage(payload, runInstanceMock.object); assert.ok(capturedCoverage); - assert.strictEqual(capturedCoverage!.uri.fsPath, '/path/to/file.py'); + assert.strictEqual(capturedCoverage!.uri.fsPath, Uri.file('/path/to/file.py').fsPath); }); test('should return detailed coverage map with correct keys', () => { From dd9288fc7ca1e6240e34f725b023fb15f3cceb7a Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 30 Nov 2025 20:48:26 -0800 Subject: [PATCH 06/10] learnings --- .github/instructions/testing-workflow.instructions.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index 4478c07b5e6c..948886a59635 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -575,5 +575,6 @@ envConfig.inspect ## 🧠 Agent Learnings -- When mocking `testController.createTestItem()` in unit tests, use `typemoq.It.isAny()` for parameters when testing handler behavior (not ID/label generation logic), but consider using specific matchers (e.g., `It.is((id: string) => id.startsWith('_error_'))`) when the actual values being passed are important for correctness - this balances test precision with maintainability (1) +- When mocking `testController.createTestItem()` in unit tests, use `typemoq.It.isAny()` for parameters when testing handler behavior (not ID/label generation logic), but consider using specific matchers (e.g., `It.is((id: string) => id.startsWith('_error_'))`) when the actual values being passed are important for correctness - this balances test precision with maintainability (2) - Remove unused variables from test code immediately - leftover tracking variables like `validationCallCount` that aren't referenced indicate dead code that should be simplified (1) +- Use `Uri.file(path).fsPath` for both sides of path comparisons in tests to ensure cross-platform compatibility - Windows converts forward slashes to backslashes automatically (1) From 2ad6cde996500fdb715788b58d83a7c3f038c632 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Wed, 3 Dec 2025 09:43:08 -0800 Subject: [PATCH 07/10] test fixes --- .../testing/common/testingAdapter.test.ts | 132 +++++++++--------- 1 file changed, 63 insertions(+), 69 deletions(-) diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index 83e57827f96b..4334ed0b6b77 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -161,11 +161,10 @@ suite('End to End Tests: test adapters', () => { resultResolver = new PythonResultResolver(testController, unittestProvider, workspaceUri); let callCount = 0; // const deferredTillEOT = createTestingDeferred(); - resultResolver.resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; - return Promise.resolve(); }; // set workspace to test workspace folder and set up settings @@ -202,11 +201,10 @@ suite('End to End Tests: test adapters', () => { }; resultResolver = new PythonResultResolver(testController, unittestProvider, workspaceUri); let callCount = 0; - resultResolver.resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; - return Promise.resolve(); }; // set settings to work for the given workspace @@ -242,10 +240,9 @@ suite('End to End Tests: test adapters', () => { workspaceUri = Uri.parse(rootPathSmallWorkspace); resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver.resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = (payload, _token?) => { callCount = callCount + 1; actualData = payload; - return Promise.resolve(); }; // run pytest discovery const discoveryAdapter = new PytestTestDiscoveryAdapter(configService, resultResolver, envVarsService); @@ -291,11 +288,10 @@ suite('End to End Tests: test adapters', () => { resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver.resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; - return Promise.resolve(); }; // run pytest discovery const discoveryAdapter = new PytestTestDiscoveryAdapter(configService, resultResolver, envVarsService); @@ -375,11 +371,10 @@ suite('End to End Tests: test adapters', () => { resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver.resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; - return Promise.resolve(); }; // run pytest discovery const discoveryAdapter = new PytestTestDiscoveryAdapter(configService, resultResolver, envVarsService); @@ -446,11 +441,10 @@ suite('End to End Tests: test adapters', () => { }; resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); let callCount = 0; - resultResolver.resolveDiscovery = async (payload, _token?) => { + resultResolver.resolveDiscovery = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; actualData = payload; - return Promise.resolve(); }; // run pytest discovery const discoveryAdapter = new PytestTestDiscoveryAdapter(configService, resultResolver, envVarsService); @@ -480,22 +474,23 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. try { - assert.strictEqual( - payload.status, - 'success', - `Expected status to be 'success', instead status is ${payload.status}`, - ); - assert.ok(payload.result, 'Expected results to be present'); + if ('status' in payload) { + assert.strictEqual( + payload.status, + 'success', + `Expected status to be 'success', instead status is ${payload.status}`, + ); + assert.ok(payload.result, 'Expected results to be present'); + } } catch (err) { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; // set workspace to test workspace folder @@ -554,22 +549,25 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. try { - const validStatuses = ['subtest-success', 'subtest-failure']; - assert.ok( - validStatuses.includes(payload.status), - `Expected status to be one of ${validStatuses.join(', ')}, but instead status is ${payload.status}`, - ); - assert.ok(payload.result, 'Expected results to be present'); + if ('status' in payload) { + const validStatuses = ['subtest-success', 'subtest-failure']; + assert.ok( + validStatuses.includes(payload.status), + `Expected status to be one of ${validStatuses.join(', ')}, but instead status is ${ + payload.status + }`, + ); + assert.ok(payload.result, 'Expected results to be present'); + } } catch (err) { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; // set workspace to test workspace folder @@ -625,22 +623,23 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. try { - assert.strictEqual( - payload.status, - 'success', - `Expected status to be 'success', instead status is ${payload.status}`, - ); - assert.ok(payload.result, 'Expected results to be present'); + if ('status' in payload) { + assert.strictEqual( + payload.status, + 'success', + `Expected status to be 'success', instead status is ${payload.status}`, + ); + assert.ok(payload.result, 'Expected results to be present'); + } } catch (err) { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; // set workspace to test workspace folder workspaceUri = Uri.parse(rootPathSmallWorkspace); @@ -707,7 +706,7 @@ suite('End to End Tests: test adapters', () => { test('Unittest execution with coverage, small workspace', async () => { // result resolver and saved data for assertions resultResolver = new PythonResultResolver(testController, unittestProvider, workspaceUri); - resultResolver._resolveCoverage = async (payload, _token?) => { + resultResolver._resolveCoverage = (payload, _token?) => { assert.strictEqual(payload.cwd, rootPathCoverageWorkspace, 'Expected cwd to be the workspace folder'); assert.ok(payload.result, 'Expected results to be present'); const simpleFileCov = payload.result[`${rootPathCoverageWorkspace}/even.py`]; @@ -717,7 +716,6 @@ suite('End to End Tests: test adapters', () => { assert.strictEqual(simpleFileCov.lines_missed.length, 1, 'Expected 3 lines to be missed in even.py'); assert.strictEqual(simpleFileCov.executed_branches, 1, 'Expected 1 branch to be executed in even.py'); assert.strictEqual(simpleFileCov.total_branches, 2, 'Expected 2 branches in even.py'); - return Promise.resolve(); }; // set workspace to test workspace folder @@ -757,7 +755,7 @@ suite('End to End Tests: test adapters', () => { test('pytest coverage execution, small workspace', async () => { // result resolver and saved data for assertions resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); - resultResolver._resolveCoverage = async (payload, _runInstance?) => { + resultResolver._resolveCoverage = (payload, _runInstance?) => { assert.strictEqual(payload.cwd, rootPathCoverageWorkspace, 'Expected cwd to be the workspace folder'); assert.ok(payload.result, 'Expected results to be present'); const simpleFileCov = payload.result[`${rootPathCoverageWorkspace}/even.py`]; @@ -767,8 +765,6 @@ suite('End to End Tests: test adapters', () => { assert.strictEqual(simpleFileCov.lines_missed.length, 1, 'Expected 3 lines to be missed in even.py'); assert.strictEqual(simpleFileCov.executed_branches, 1, 'Expected 1 branch to be executed in even.py'); assert.strictEqual(simpleFileCov.total_branches, 2, 'Expected 2 branches in even.py'); - - return Promise.resolve(); }; // set workspace to test workspace folder workspaceUri = Uri.parse(rootPathCoverageWorkspace); @@ -811,22 +807,23 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveExecution = async (payload, _token?) => { + resultResolver.resolveExecution = (payload, _token?) => { traceLog(`resolveDiscovery ${payload}`); callCount = callCount + 1; // the payloads that get to the _resolveExecution are all data and should be successful. try { - assert.strictEqual( - payload.status, - 'success', - `Expected status to be 'success', instead status is ${payload.status}`, - ); - assert.ok(payload.result, 'Expected results to be present'); + if ('status' in payload) { + assert.strictEqual( + payload.status, + 'success', + `Expected status to be 'success', instead status is ${payload.status}`, + ); + assert.ok(payload.result, 'Expected results to be present'); + } } catch (err) { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; // set workspace to test workspace folder @@ -878,7 +875,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveDiscovery = async (data, _token?) => { + resultResolver.resolveDiscovery = (data, _token?) => { // do the following asserts for each time resolveExecution is called, should be called once per test. callCount = callCount + 1; traceLog(`unittest discovery adapter seg fault error handling \n ${JSON.stringify(data)}`); @@ -903,7 +900,6 @@ suite('End to End Tests: test adapters', () => { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; // set workspace to test workspace folder @@ -931,7 +927,7 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveDiscovery = async (data, _token?) => { + resultResolver.resolveDiscovery = (data, _token?) => { // do the following asserts for each time resolveExecution is called, should be called once per test. callCount = callCount + 1; traceLog(`add one to call count, is now ${callCount}`); @@ -961,7 +957,6 @@ suite('End to End Tests: test adapters', () => { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; // run pytest discovery const discoveryAdapter = new PytestTestDiscoveryAdapter(configService, resultResolver, envVarsService); @@ -984,22 +979,24 @@ suite('End to End Tests: test adapters', () => { let callCount = 0; let failureOccurred = false; let failureMsg = ''; - resultResolver.resolveExecution = async (data, _token?) => { + resultResolver.resolveExecution = (data, _token?) => { // do the following asserts for each time resolveExecution is called, should be called once per test. console.log(`pytest execution adapter seg fault error handling \n ${JSON.stringify(data)}`); callCount = callCount + 1; try { - if (data.status === 'error') { - assert.ok(data.error, "Expected errors in 'error' field"); - } else { - const indexOfTest = JSON.stringify(data.result).search('error'); - assert.notDeepEqual( - indexOfTest, - -1, - 'If payload status is not error then the individual tests should be marked as errors. This should occur on windows machines.', - ); + if ('status' in data) { + if (data.status === 'error') { + assert.ok(data.error, "Expected errors in 'error' field"); + } else { + const indexOfTest = JSON.stringify(data.result).search('error'); + assert.notDeepEqual( + indexOfTest, + -1, + 'If payload status is not error then the individual tests should be marked as errors. This should occur on windows machines.', + ); + } + assert.ok(data.result, 'Expected results to be present'); } - assert.ok(data.result, 'Expected results to be present'); // make sure the testID is found in the results const indexOfTest = JSON.stringify(data).search( 'test_seg_fault.py::TestSegmentationFault::test_segfault', @@ -1009,7 +1006,6 @@ suite('End to End Tests: test adapters', () => { failureMsg = err ? (err as Error).toString() : ''; failureOccurred = true; } - return Promise.resolve(); }; const testId = `${rootPathErrorWorkspace}/test_seg_fault.py::TestSegmentationFault::test_segfault`; @@ -1091,19 +1087,17 @@ suite('End to End Tests: test adapters', () => { // Wrap the _resolveExecution function to measure performance const original_resolveExecution = resultResolver.resolveExecution.bind(resultResolver); - resultResolver.resolveExecution = async (payload, runInstance) => { + resultResolver.resolveExecution = (payload, runInstance) => { const startTime = performance.now(); callCount++; // Call the actual implementation - await original_resolveExecution(payload, runInstance); + original_resolveExecution(payload, runInstance); const endTime = performance.now(); const callTime = endTime - startTime; callTimes.push(callTime); totalCallTime += callTime; - - return Promise.resolve(); }; // ================================================================ From 4ea0ad4aef6fd2d401be5b94958253bee8f70b42 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Wed, 3 Dec 2025 10:11:34 -0800 Subject: [PATCH 08/10] fix test --- src/test/testing/common/testingAdapter.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index 4334ed0b6b77..478e9dd85744 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -1158,7 +1158,8 @@ suite('End to End Tests: test adapters', () => { } // 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}`] = { + // Use test IDs that actually exist in our mock setup (test_0_0 through test_0_9) + testResults[`test_0_${i % testFunctionsPerFile}`] = { test: `test_method[${i}]`, outcome: 'success', message: null, From 7169c54db8d5c5f8bdcb09401d32bcaff2de0b0e Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 5 Dec 2025 11:24:05 -0800 Subject: [PATCH 09/10] remove instructions for refactor --- .../testController/resultHandlerRefactor.md | 1014 ----------------- 1 file changed, 1014 deletions(-) delete mode 100644 src/client/testing/testController/resultHandlerRefactor.md diff --git a/src/client/testing/testController/resultHandlerRefactor.md b/src/client/testing/testController/resultHandlerRefactor.md deleted file mode 100644 index b3614f94b52b..000000000000 --- a/src/client/testing/testController/resultHandlerRefactor.md +++ /dev/null @@ -1,1014 +0,0 @@ -# Result Resolver Refactoring Specification - -## Overview - -Refactor `PythonResultResolver` to separate stateful concerns (ID mappings) from stateless processing logic (payload handling). This improves testability, clarity, and sets the foundation for future project-based architecture. - -## Current Problems - -The `PythonResultResolver` class conflates multiple responsibilities: -1. **Persistent state**: ID mappings between Python test IDs and VS Code TestItem IDs -2. **Discovery processing**: Building TestItem trees from JSON payloads -3. **Execution processing**: Updating TestRun instances from execution results -4. **Coverage processing**: Handling coverage data -5. **Error handling**: Creating error nodes - -This mixing of state and processing makes it: -- Hard to reason about lifecycle and ownership -- Difficult to test in isolation -- Unclear what state persists vs. what is transient -- Prone to state conflicts if multiple operations run concurrently - -## New Design: Separation of Concerns - -### Ownership Model Summary - -**Singleton (One per Extension):** -- `TestDiscoveryHandler` - stateless, shared by all workspaces -- `TestExecutionHandler` - stateless, shared by all workspaces -- `TestCoverageHandler` - stateless, shared by all workspaces - -**Per-Workspace:** -- `PythonResultResolver` - facade that coordinates components -- `TestItemIndex` - stateful, stores ID mappings for this workspace's tests - -**Shared References:** -- `TestController` - one per extension, passed to handlers as parameter -- `TestRun` - one per execution request, passed to handlers as parameter - -**Why This Works:** -- Handlers are pure functions with no instance state → safe to share -- All state is passed as parameters or stored in caller (resolver/index) -- Each workspace gets its own index, but all use the same handler logic - -### State Lifecycle: Where Test ID Mappings Live - -The `TestItemIndex` is **the only stateful component** in the new design. Here's its complete lifecycle: - -#### 1. Creation (Workspace Activation) -```typescript -// In PythonTestController.activate() -const resultResolver = new PythonResultResolver( - this.testController, - testProvider, - workspace.uri -); - -// Inside PythonResultResolver constructor -constructor(testController, testProvider, workspaceUri) { - this.testItemIndex = new TestItemIndex(); // ← CREATED HERE - // Initially empty: no test IDs registered yet -} -``` - -#### 2. Population (Discovery Phase) -```typescript -// Discovery flow: -Python subprocess → DiscoveryAdapter → PythonResultResolver.resolveDiscovery() - → TestDiscoveryHandler.processDiscovery() - -// Inside TestDiscoveryHandler.processDiscovery() -testItemIndex.clear(); // Wipe old mappings - -for each discovered test { - // Create VS Code TestItem - const testItem = testController.createTestItem(test.id_, test.name, uri); - - // Register in index (WRITES STATE) - testItemIndex.registerTestItem( - runId: test.runID, // e.g., "test_file.py::test_example" - vsId: test.id_, // e.g., "test_file.py::test_example" - testItem: testItem // Reference to VS Code TestItem object - ); -} - -// Now index contains: -// runIdToTestItem: { "test_file.py::test_example" → TestItem } -// runIdToVSid: { "test_file.py::test_example" → "test_file.py::test_example" } -// vsIdToRunId: { "test_file.py::test_example" → "test_file.py::test_example" } -``` - -#### 3. Query (Execution Preparation) -```typescript -// In WorkspaceTestAdapter.executeTests() -// User selected some tests to run, need to convert VS Code IDs → Python IDs - -testCaseNodes.forEach((node) => { - // READS STATE from index (via resolver getter) - const runId = resultResolver.vsIdToRunId.get(node.id); - // ↑ This delegates to: testItemIndex.getRunId(node.id) - - if (runId) { - testCaseIds.push(runId); // Send to Python subprocess - } -}); -``` - -#### 4. Lookup (Execution Results Processing) -```typescript -// Execution flow: -Python subprocess → ExecutionAdapter → PythonResultResolver.resolveExecution() - → TestExecutionHandler.processExecution() - -// Inside TestExecutionHandler.processExecution() -for each test result in payload { - const runId = "test_file.py::test_example"; // From Python - - // READS STATE from index - const testItem = testItemIndex.getTestItem(runId, testController); - // ↑ Looks up: runIdToTestItem.get("test_file.py::test_example") → TestItem - - if (testItem) { - runInstance.passed(testItem); // Update VS Code UI - } -} -``` - -#### 5. Cleanup (Periodic or On Demand) -```typescript -// When tests are deleted/modified without full rediscovery -resultResolver.cleanupStaleReferences(); -// ↓ Delegates to: -testItemIndex.cleanupStaleReferences(testController); - -// Removes mappings for TestItems that no longer exist in the tree -``` - -#### 6. Disposal (Workspace Closed) -```typescript -// When workspace is removed or extension deactivates -// PythonResultResolver gets garbage collected -// → testItemIndex gets garbage collected -// → Maps are freed -``` - -### Key Insight: Index is the "Glue" - -``` -┌─────────────────────────────────────────────────────────────┐ -│ DISCOVERY PHASE │ -├─────────────────────────────────────────────────────────────┤ -│ Python: "test_file.py::test_example" │ -│ ↓ │ -│ TestDiscoveryHandler: Creates TestItem │ -│ ↓ │ -│ TestItemIndex: Stores mapping │ -│ runId "test_file.py::test_example" → TestItem instance │ -└─────────────────────────────────────────────────────────────┘ - ↓ - (Index persists) - ↓ -┌─────────────────────────────────────────────────────────────┐ -│ EXECUTION PHASE │ -├─────────────────────────────────────────────────────────────┤ -│ Python: "test_file.py::test_example" succeeded │ -│ ↓ │ -│ TestExecutionHandler: Needs TestItem to update │ -│ ↓ │ -│ TestItemIndex: Retrieves mapping │ -│ runId "test_file.py::test_example" → TestItem instance │ -│ ↓ │ -│ runInstance.passed(testItem) → UI updates ✓ │ -└─────────────────────────────────────────────────────────────┘ -``` - -The index is **persistent state** that bridges discovery and execution, while handlers are **stateless processors** that operate on that state. - -### Component 1: `TestItemIndex` (Stateful - Per Workspace/Adapter) - -**Purpose**: Maintains persistent ID mappings between Python test IDs and VS Code TestItems. - -**Ownership**: **One instance per workspace**. Created by `PythonResultResolver` constructor. - -**Lifecycle**: -- **Created**: When `PythonResultResolver` is instantiated (during workspace activation) -- **Populated**: During discovery - each discovered test registers its mappings -- **Queried**: During execution - to look up TestItems by Python run ID -- **Cleared**: When discovery runs again (fresh start) or workspace is disposed -- **Cleaned**: Periodically to remove stale references to deleted tests - -**State Management**: -```typescript -// Discovery phase - WRITES to index -TestDiscoveryHandler.processDiscovery() { - testItemIndex.clear(); // Start fresh - for each discovered test: - testItemIndex.registerTestItem(test.runID, test.id_, testItem); -} - -// Execution phase - READS from index -TestExecutionHandler.processExecution() { - for each test result: - testItem = testItemIndex.getTestItem(runId); // Lookup! - runInstance.passed/failed/errored(testItem); -} -``` - -**Responsibilities**: -- Store bidirectional mappings: `runId ↔ TestItem`, `runId ↔ vsId` -- Provide efficient lookup methods -- Clean up stale references when tests are removed -- Validate TestItem references are still in the tree - -**Location**: `src/client/testing/testController/common/testItemIndex.ts` - -**Interface**: -```typescript -export class TestItemIndex { - // THE STATE - these maps persist across discovery and execution - private runIdToTestItem: Map; - private runIdToVSid: Map; - private vsIdToRunId: Map; - - constructor(); - - /** - * Register a test item with its Python run ID and VS Code ID - * Called during DISCOVERY to populate the index - */ - registerTestItem(runId: string, vsId: string, testItem: TestItem): void; - - /** - * Get TestItem by Python run ID (with validation and fallback strategies) - * Called during EXECUTION to look up tests - */ - getTestItem(runId: string, testController: TestController): TestItem | undefined; - - /** - * Get Python run ID from VS Code ID - * Called by WorkspaceTestAdapter.executeTests() to convert selected tests to Python IDs - */ - getRunId(vsId: string): string | undefined; - - /** - * Get VS Code ID from Python run ID - */ - getVSId(runId: string): string | undefined; - - /** - * Check if a TestItem reference is still valid in the tree - */ - isTestItemValid(testItem: TestItem, testController: TestController): boolean; - - /** - * Remove all mappings - * Called at the start of discovery to ensure clean state - */ - clear(): void; - - /** - * Clean up stale references that no longer exist in the test tree - * Called after test tree modifications - */ - cleanupStaleReferences(testController: TestController): void; -} -``` - -### Component 2: `TestDiscoveryHandler` (Stateless - Shared) - -**Purpose**: Processes discovery payloads and builds/updates the TestItem tree. - -**Ownership**: **One shared instance** created at the module/service level, reused by all resolvers/adapters. - -**Responsibilities**: -- Parse `DiscoveredTestPayload` and create/update TestItems -- Call `TestItemIndex.registerTestItem()` for each discovered test -- Handle discovery errors and create error nodes -- Populate test tree structure - -**Location**: `src/client/testing/testController/common/testDiscoveryHandler.ts` - -**Interface**: -```typescript -export class TestDiscoveryHandler { - /** - * Process discovery payload and update test tree - * Pure function - no instance state used - */ - processDiscovery( - payload: DiscoveredTestPayload, - testController: TestController, - testItemIndex: TestItemIndex, - workspaceUri: Uri, - testProvider: TestProvider, - token?: CancellationToken - ): void; - - /** - * Create an error node for discovery failures - */ - createErrorNode( - testController: TestController, - workspaceUri: Uri, - message: string, - testProvider: TestProvider - ): TestItem; -} -``` - -### Component 3: `TestExecutionHandler` (Stateless - Shared) - -**Purpose**: Processes execution payloads and updates TestRun instances. - -**Ownership**: **One shared instance** created at the module/service level, reused by all resolvers/adapters. - -**Responsibilities**: -- Parse `ExecutionTestPayload` and update TestRun with results (passed/failed/skipped/errored) -- Look up TestItems using `TestItemIndex` -- Handle subtests (create child TestItems dynamically) -- Process test outcomes and create TestMessages - -**Location**: `src/client/testing/testController/common/testExecutionHandler.ts` - -**Interface**: -```typescript -export class TestExecutionHandler { - /** - * Process execution payload and update test run - * Pure function - no instance state used - * Returns subtest statistics for caller to manage - */ - processExecution( - payload: ExecutionTestPayload, - runInstance: TestRun, - testItemIndex: TestItemIndex, - testController: TestController - ): Map; - - /** - * Handle a single test result based on outcome - */ - private handleTestOutcome( - runId: string, - testItem: any, - runInstance: TestRun, - testItemIndex: TestItemIndex, - testController: TestController, - subtestStats: Map - ): void; -} - -export interface SubtestStats { - passed: number; - failed: number; -} -``` - -### Component 4: `TestCoverageHandler` (Stateless - Shared) - -**Purpose**: Processes coverage payloads and creates coverage objects. - -**Ownership**: **One shared instance** created at the module/service level, reused by all resolvers/adapters. - -**Responsibilities**: -- Parse `CoveragePayload` and create `FileCoverage` objects -- Generate detailed coverage information -- Return coverage data for caller to store/use - -**Location**: `src/client/testing/testController/common/testCoverageHandler.ts` - -**Interface**: -```typescript -export class TestCoverageHandler { - /** - * Process coverage payload - * Pure function - returns coverage data without storing it - */ - processCoverage( - payload: CoveragePayload, - runInstance: TestRun - ): Map; - - /** - * Create FileCoverage object from metrics - */ - private createFileCoverage( - uri: Uri, - metrics: FileCoverageMetrics - ): FileCoverage; - - /** - * Create detailed coverage array for a file - */ - private createDetailedCoverage( - linesCovered: number[], - linesMissed: number[] - ): FileCoverageDetail[]; -} -``` - -### Component 5: `PythonResultResolver` (Adapter/Facade - Maintained for Compatibility) - -**Purpose**: Maintains backward compatibility during transition. Delegates to new components. - -**Ownership**: **One instance per workspace** (current model). References shared handler instances. - -**Responsibilities**: -- Wrap new components to maintain existing API -- Eventually can be deprecated once all callers migrate - -**Location**: `src/client/testing/testController/common/resultResolver.ts` (modified) - -**Interface**: -```typescript -export class PythonResultResolver implements ITestResultResolver { - private testItemIndex: TestItemIndex; // Per-workspace instance - private testController: TestController; // Shared reference - private testProvider: TestProvider; // Configuration - private workspaceUri: Uri; // Configuration - - // References to shared singleton handlers - private static discoveryHandler: TestDiscoveryHandler = new TestDiscoveryHandler(); - private static executionHandler: TestExecutionHandler = new TestExecutionHandler(); - private static coverageHandler: TestCoverageHandler = new TestCoverageHandler(); - - // Expose for backward compatibility (WorkspaceTestAdapter accesses these) - public get runIdToTestItem(): Map { - return this.testItemIndex.runIdToTestItem; - } - public get runIdToVSid(): Map { - return this.testItemIndex.runIdToVSid; - } - public get vsIdToRunId(): Map { - return this.testItemIndex.vsIdToRunId; - } - public subTestStats: Map; - public detailedCoverageMap: Map; - - constructor( - testController: TestController, - testProvider: TestProvider, - workspaceUri: Uri - ) { - this.testController = testController; - this.testProvider = testProvider; - this.workspaceUri = workspaceUri; - this.testItemIndex = new TestItemIndex(); // Per-workspace state - this.subTestStats = new Map(); - this.detailedCoverageMap = new Map(); - } - - public resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void { - PythonResultResolver.discoveryHandler.processDiscovery( - payload, - this.testController, - this.testItemIndex, - this.workspaceUri, - this.testProvider, - token - ); - } - - public resolveExecution(payload: ExecutionTestPayload | CoveragePayload, runInstance: TestRun): void { - if ('coverage' in payload) { - const coverageMap = PythonResultResolver.coverageHandler.processCoverage(payload, runInstance); - this.detailedCoverageMap = coverageMap; - } else { - this.subTestStats = PythonResultResolver.executionHandler.processExecution( - payload, - runInstance, - this.testItemIndex, - this.testController - ); - } - } - - // Delegate cleanup to index - public cleanupStaleReferences(): void { - this.testItemIndex.cleanupStaleReferences(this.testController); - } -} -``` - -## Migration Strategy - -### Phase 1: Extract `TestItemIndex` -**Goal**: Separate ID mapping state from processing logic - -**Steps**: -1. Create `src/client/testing/testController/common/testItemIndex.ts` -2. Move mapping-related methods from `PythonResultResolver`: - - `runIdToTestItem`, `runIdToVSid`, `vsIdToRunId` maps - - `findTestItemByIdEfficient()` - - `isTestItemValid()` - - `cleanupStaleReferences()` -3. Update `PythonResultResolver` to use `TestItemIndex` internally -4. Maintain backward compatibility with existing API -5. Add unit tests for `TestItemIndex` - -**Files Changed**: -- New: `testItemIndex.ts` -- Modified: `resultResolver.ts` - -**Tests**: -- Test ID registration and lookup -- Test stale reference cleanup -- Test validation logic - -### Phase 2: Extract `TestDiscoveryHandler` -**Goal**: Separate discovery processing into stateless handler - -**Steps**: -1. Create `src/client/testing/testController/common/testDiscoveryHandler.ts` -2. Move discovery-related methods from `PythonResultResolver`: - - `_resolveDiscovery()` - - Error node creation logic - - Test tree population logic (from `utils.ts`) -3. Update `PythonResultResolver.resolveDiscovery()` to delegate to handler -4. Add unit tests for `TestDiscoveryHandler` - -**Files Changed**: -- New: `testDiscoveryHandler.ts` -- Modified: `resultResolver.ts` -- Modified: `utils.ts` (move `populateTestTree` to handler) - -**Tests**: -- Test discovery payload processing -- Test error handling -- Test TestItem creation and tree building - -### Phase 3: Extract `TestExecutionHandler` -**Goal**: Separate execution processing into stateless handler - -**Steps**: -1. Create `src/client/testing/testController/common/testExecutionHandler.ts` -2. Move execution-related methods from `PythonResultResolver`: - - `_resolveExecution()` - - `handleTestError()`, `handleTestFailure()`, `handleTestSuccess()`, `handleTestSkipped()` - - `handleSubtestFailure()`, `handleSubtestSuccess()` -3. Update handler to return subtest stats instead of storing them -4. Update `PythonResultResolver.resolveExecution()` to delegate to handler -5. Add unit tests for `TestExecutionHandler` - -**Files Changed**: -- New: `testExecutionHandler.ts` -- Modified: `resultResolver.ts` - -**Tests**: -- Test each outcome type (error, failure, success, skipped) -- Test subtest handling -- Test message creation with tracebacks - -### Phase 4: Extract `TestCoverageHandler` -**Goal**: Separate coverage processing into stateless handler - -**Steps**: -1. Create `src/client/testing/testController/common/testCoverageHandler.ts` -2. Move coverage-related methods from `PythonResultResolver`: - - `_resolveCoverage()` - - Coverage object creation logic -3. Update handler to return coverage data instead of storing it -4. Update `PythonResultResolver.resolveExecution()` to delegate to handler and store results -5. Add unit tests for `TestCoverageHandler` - -**Files Changed**: -- New: `testCoverageHandler.ts` -- Modified: `resultResolver.ts` - -**Tests**: -- Test coverage payload processing -- Test FileCoverage creation -- Test detailed coverage generation - -### Phase 5: Update `PythonResultResolver` to Pure Facade -**Goal**: Simplify resolver to only coordinate components - -**Steps**: -1. Remove all processing logic from `PythonResultResolver` -2. Keep only delegation and backward compatibility methods -3. Update constructor to instantiate handler components -4. Ensure all existing tests still pass - -**Files Changed**: -- Modified: `resultResolver.ts` - -### Phase 6: Direct Migration (Optional - Future) -**Goal**: Update callers to use handlers directly instead of through resolver - -**Steps**: -1. Update `WorkspaceTestAdapter` to use handlers directly -2. Update discovery/execution adapters if needed -3. Eventually deprecate `PythonResultResolver` once all callers migrated - -**Files Changed**: -- Modified: `workspaceTestAdapter.ts` -- Modified: `pytestDiscoveryAdapter.ts`, `unittestDiscoveryAdapter.ts` -- Modified: `pytestExecutionAdapter.ts`, `unittestExecutionAdapter.ts` - -## Benefits - -### Immediate Benefits -1. **Testability**: Each component can be unit tested in isolation with simple inputs/outputs -2. **Clarity**: Clear separation between state (index) and processing (handlers) -3. **Maintainability**: Smaller, focused classes are easier to understand and modify -4. **Type Safety**: Clearer interfaces and types for each concern - -### Future Benefits (Project-based Architecture) -1. **Concurrency**: Stateless handlers can be safely shared across projects -2. **Scalability**: Each project gets its own `TestItemIndex`, handlers are shared -3. **Flexibility**: Easy to add new processing logic without modifying state management -4. **Migration Path**: Clean abstractions make it easier to introduce project adapters - -##################################################### - -# Parts to do later -> - -## Testing Strategy - -### Unit Tests - -**`TestItemIndex`**: -- Test registration and lookup operations -- Test stale reference detection -- Test cleanup operations -- Test edge cases (missing items, invalid references) - -**`TestDiscoveryHandler`**: -- Test parsing valid discovery payloads -- Test error payload handling -- Test tree building with various structures -- Test edge cases (null tests, empty payloads) - -**`TestExecutionHandler`**: -- Test each outcome type (success, failure, error, skip) -- Test subtest creation and statistics -- Test message creation with locations -- Test edge cases (missing items, invalid IDs) - -**`TestCoverageHandler`**: -- Test coverage payload parsing -- Test FileCoverage creation -- Test detailed coverage generation -- Test branch coverage vs. line coverage - -### Integration Tests - -**End-to-end discovery flow**: -- Verify discovery adapters → handlers → TestController updates work correctly -- Verify index is populated correctly during discovery - -**End-to-end execution flow**: -- Verify execution adapters → handlers → TestRun updates work correctly -- Verify index lookups work during execution - -### Regression Tests - -- Run full test suite to ensure no behavioral changes -- Verify all existing discovery/execution scenarios still work -- Verify coverage functionality unchanged - -## Risks and Mitigations - -### Risk: Breaking Changes -**Mitigation**: Maintain `PythonResultResolver` as compatibility facade during transition. All existing callers continue to work unchanged. - -### Risk: Performance Regression -**Mitigation**: Handlers are pure functions with no additional overhead. Index operations remain O(1) lookups. Profile before/after to verify. - -### Risk: Incomplete State Migration -**Mitigation**: Phase 1 focuses entirely on extracting index with full backward compatibility. Each phase is independently testable. - -### Risk: Subtest Stats State Management -**Mitigation**: Return stats from handler rather than storing. Let caller (resolver or adapter) decide how to manage this transient state. - -## Success Criteria - -1. ✅ All existing tests pass without modification -2. ✅ New unit tests achieve >90% coverage for new components -3. ✅ No performance degradation in discovery/execution -4. ✅ Code is more modular and testable -5. ✅ Clear path forward for project-based architecture -6. ✅ No breaking changes to external APIs - -## Information Flow - -### Discovery Flow: From Python Subprocess to Test Tree - -``` -┌─────────────────────────────────────────────────────────────────────────┐ -│ 1. USER ACTION: Refresh Tests / Auto-discovery Trigger │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 2. PythonTestController.refreshTestData() │ -│ - Determines which workspace(s) to refresh │ -│ - Calls refreshSingleWorkspace(uri) or refreshAllWorkspaces() │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 3. PythonTestController.discoverTestsForProvider() │ -│ - Gets WorkspaceTestAdapter for workspace │ -│ - Calls testAdapter.discoverTests(...) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 4. WorkspaceTestAdapter.discoverTests() │ -│ - Calls discoveryAdapter.discoverTests(uri, factory, token) │ -│ - Waits for discovery to complete │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 5. [Pytest|Unittest]DiscoveryAdapter.discoverTests() │ -│ - Sets up named pipe for IPC │ -│ - Spawns Python subprocess with discovery script │ -│ - Subprocess runs: python_files/vscode_pytest/run_pytest_script.py │ -│ or python_files/unittestadapter/discovery.py │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 6. PYTHON SUBPROCESS: Discovery Script │ -│ - Collects tests using pytest/unittest framework │ -│ - Builds DiscoveredTestPayload JSON: │ -│ { │ -│ "status": "success", │ -│ "cwd": "/workspace/path", │ -│ "tests": { │ -│ "rootid": ".", │ -│ "root": "/workspace/path", │ -│ "parents": [...], │ -│ "tests": [ │ -│ { │ -│ "name": "test_example", │ -│ "path": "./test_file.py", │ -│ "type_": "test", │ -│ "id_": "test_file.py::test_example", │ -│ "runID": "test_file.py::test_example", │ -│ "lineno": 10 │ -│ } │ -│ ] │ -│ } │ -│ } │ -│ - Sends JSON over named pipe │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 7. Discovery Adapter: Receives IPC Data │ -│ - Named pipe listener receives JSON chunks │ -│ - Parses complete JSON into DiscoveredTestPayload │ -│ - Calls: resultResolver.resolveDiscovery(payload, token) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 8. PythonResultResolver.resolveDiscovery() [FACADE] │ -│ - Validates payload is not null │ -│ - Delegates to: │ -│ this.discoveryHandler.processDiscovery( │ -│ payload, │ -│ this.testController, │ -│ this.testItemIndex, │ -│ this.workspaceUri, │ -│ this.testProvider, │ -│ token │ -│ ) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 9. TestDiscoveryHandler.processDiscovery() [STATELESS] │ -│ - Checks payload.status for errors │ -│ - If errors: creates error node and adds to TestController │ -│ - If success: processes payload.tests │ -│ - Calls populateTestTree() to build TestItem hierarchy │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 10. TestDiscoveryHandler: Build Test Tree │ -│ - Clears testItemIndex mappings (fresh start) │ -│ - Iterates through discovered tests recursively │ -│ - For each test: │ -│ a. Creates VS Code TestItem: │ -│ testItem = testController.createTestItem( │ -│ id: test.id_, │ -│ label: test.name, │ -│ uri: Uri.file(test.path) │ -│ ) │ -│ b. Sets TestItem properties (range, canResolveChildren, etc.) │ -│ c. Registers in index: │ -│ testItemIndex.registerTestItem( │ -│ runId: test.runID, │ -│ vsId: test.id_, │ -│ testItem: testItem │ -│ ) │ -│ d. Adds TestItem to parent or TestController.items │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 11. TestItemIndex.registerTestItem() [STATEFUL] │ -│ - Stores mappings: │ -│ runIdToTestItem.set(runId, testItem) │ -│ runIdToVSid.set(runId, vsId) │ -│ vsIdToRunId.set(vsId, runId) │ -│ - These mappings persist for execution phase │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 12. VS CODE TEST EXPLORER: Tree Updated │ -│ - TestController.items now contains full test hierarchy │ -│ - Test Explorer UI refreshes to show discovered tests │ -│ - Tests are ready to be run │ -└─────────────────────────────────────────────────────────────────────────┘ - - -### Execution Flow: From Run Request to Result Updates - -┌─────────────────────────────────────────────────────────────────────────┐ -│ 1. USER ACTION: Run/Debug Tests │ -│ - User clicks run icon, uses command palette, or keyboard shortcut │ -│ - VS Code creates TestRunRequest with selected TestItems │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 2. PythonTestController.runTests(request, token) │ -│ - Gets workspaces that contain selected tests │ -│ - Creates TestRun instance from TestController │ -│ - Calls runTestsForWorkspace() for each workspace │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 3. PythonTestController.runTestsForWorkspace() │ -│ - Filters TestItems belonging to this workspace │ -│ - Gets WorkspaceTestAdapter for workspace │ -│ - Calls testAdapter.executeTests(...) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 4. WorkspaceTestAdapter.executeTests() │ -│ - Collects all test case nodes from selected items │ -│ - For each test, looks up Python runID: │ -│ runId = resultResolver.vsIdToRunId.get(node.id) │ -│ - Calls runInstance.started(node) for each test │ -│ - Calls executionAdapter.runTests(uri, testCaseIds, ...) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 5. [Pytest|Unittest]ExecutionAdapter.runTests() │ -│ - Sets up named pipe for IPC │ -│ - Spawns Python subprocess with execution script │ -│ - Subprocess runs: python_files/vscode_pytest/run_pytest_script.py │ -│ or python_files/unittestadapter/execution.py │ -│ - Passes test IDs as arguments │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 6. PYTHON SUBPROCESS: Execution Script │ -│ - Runs selected tests using pytest/unittest framework │ -│ - Captures results in real-time │ -│ - For each test result, builds ExecutionTestPayload: │ -│ { │ -│ "result": { │ -│ "test_file.py::test_example": { │ -│ "test": "test_file.py::test_example", │ -│ "outcome": "success", // or "failure", "error", "skipped" │ -│ "message": "...", │ -│ "traceback": "...", │ -│ "subtest": null │ -│ } │ -│ } │ -│ } │ -│ - Sends JSON over named pipe (can send multiple payloads) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 7. Execution Adapter: Receives IPC Data (Streaming) │ -│ - Named pipe listener receives JSON chunks as tests complete │ -│ - For each complete JSON payload: │ -│ - Parses into ExecutionTestPayload or CoveragePayload │ -│ - Calls: resultResolver.resolveExecution(payload, runInstance) │ -│ - Updates happen in real-time as tests finish │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 8. PythonResultResolver.resolveExecution() [FACADE] │ -│ - Checks payload type: │ -│ if ('coverage' in payload): │ -│ coverageMap = coverageHandler.processCoverage(payload, run) │ -│ this.detailedCoverageMap = coverageMap │ -│ else: │ -│ subtestStats = executionHandler.processExecution( │ -│ payload, runInstance, testItemIndex, testController │ -│ ) │ -│ this.subTestStats = subtestStats │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 9a. TestExecutionHandler.processExecution() [STATELESS] │ -│ - Iterates through payload.result entries │ -│ - For each test result: │ -│ a. Looks up TestItem using index: │ -│ testItem = testItemIndex.getTestItem(runId, testController) │ -│ b. Routes to outcome-specific handler based on outcome: │ -│ - "success" → handleTestSuccess() │ -│ - "failure" → handleTestFailure() │ -│ - "error" → handleTestError() │ -│ - "skipped" → handleTestSkipped() │ -│ - "subtest-success" → handleSubtestSuccess() │ -│ - "subtest-failure" → handleSubtestFailure() │ -│ - Returns Map of subtest statistics │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 9b. TestCoverageHandler.processCoverage() [STATELESS] │ -│ - Iterates through payload.result (file paths → metrics) │ -│ - For each file: │ -│ a. Creates FileCoverage object with line/branch counts │ -│ b. Calls runInstance.addCoverage(fileCoverage) │ -│ c. Builds detailed coverage array (StatementCoverage objects) │ -│ - Returns Map │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 10. TestExecutionHandler: Update Test Results │ -│ - handleTestSuccess(runId, runInstance): │ -│ testItem = index.getTestItem(runId) │ -│ runInstance.passed(testItem) │ -│ │ -│ - handleTestFailure(runId, testData, runInstance): │ -│ testItem = index.getTestItem(runId) │ -│ message = new TestMessage(error text + traceback) │ -│ message.location = new Location(testItem.uri, testItem.range) │ -│ runInstance.failed(testItem, message) │ -│ │ -│ - handleTestError(runId, testData, runInstance): │ -│ testItem = index.getTestItem(runId) │ -│ message = new TestMessage(error details) │ -│ runInstance.errored(testItem, message) │ -│ │ -│ - handleTestSkipped(runId, runInstance): │ -│ testItem = index.getTestItem(runId) │ -│ runInstance.skipped(testItem) │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 11. TestExecutionHandler: Handle Subtests (if applicable) │ -│ - handleSubtestFailure/Success(runId, testData, runInstance): │ -│ parentTestItem = index.getTestItem(parentRunId) │ -│ subtestItem = testController.createTestItem(subtestId, ...) │ -│ parentTestItem.children.add(subtestItem) │ -│ runInstance.started(subtestItem) │ -│ runInstance.passed/failed(subtestItem, message) │ -│ - Updates subtest statistics map and returns to caller │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 12. TestItemIndex: Lookup Operations [STATEFUL] │ -│ - getTestItem(runId, testController): │ -│ 1. Try direct lookup: runIdToTestItem.get(runId) │ -│ 2. Validate item is still in tree: isTestItemValid() │ -│ 3. If stale, try vsId mapping: runIdToVSid.get(runId) │ -│ 4. Search tree using vsId │ -│ 5. Fall back to full tree search if needed │ -│ - Returns TestItem or undefined │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 13. VS CODE TEST EXPLORER: Results Updated in Real-Time │ -│ - runInstance.passed/failed/errored/skipped() calls update UI │ -│ - Test items show green checkmarks, red X's, warnings │ -│ - Error messages and tracebacks display in peek view │ -│ - Coverage decorations appear in editor (if coverage enabled) │ -│ - Duration and output appear in test results panel │ -└─────────────────────────────────────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────────────────────────────────────┐ -│ 14. Execution Complete │ -│ - Python subprocess exits │ -│ - Execution adapter resolves promise │ -│ - WorkspaceTestAdapter.executeTests() returns │ -│ - PythonTestController calls runInstance.end() │ -│ - Final telemetry sent │ -└─────────────────────────────────────────────────────────────────────────┘ -``` - -### Key Observations - -**State Management**: -- `TestItemIndex` maintains persistent mappings created during discovery -- These mappings are reused during execution for efficient lookups -- Handlers are stateless - they don't store data between calls -- Transient state (subtest stats, coverage map) returned to caller for storage - -**Data Flow Direction**: -- Discovery: Python → Adapter → Resolver → Handler → TestController (builds tree) -- Execution: Python → Adapter → Resolver → Handler → TestRun (updates results) -- Index: Populated during discovery, queried during execution - -**Separation of Concerns**: -- Adapters: IPC and subprocess management -- Resolver: Coordination and backward compatibility -- Handlers: Pure processing logic (payload → actions) -- Index: ID mapping and lookup optimization - -**Real-time Updates**: -- Execution results stream over IPC as tests complete -- Each payload is processed immediately -- UI updates happen incrementally, not in batch - -## Timeline Estimate - -- Phase 1 (TestItemIndex): 1-2 days -- Phase 2 (TestDiscoveryHandler): 2-3 days -- Phase 3 (TestExecutionHandler): 2-3 days -- Phase 4 (TestCoverageHandler): 1-2 days -- Phase 5 (Facade cleanup): 1 day -- Testing and refinement: 2-3 days - -**Total: ~2 weeks** for complete refactor with comprehensive testing From 78865d0c0e9d3b80f7f26d41a25a4303417a3c27 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 5 Dec 2025 12:37:33 -0800 Subject: [PATCH 10/10] fix to subtest issue --- .../testController/common/resultResolver.ts | 9 +- .../common/testExecutionHandler.ts | 28 +- .../testController/common/testItemIndex.ts | 23 ++ .../common/testExecutionHandler.unit.test.ts | 322 ++++++++++++++++-- 4 files changed, 337 insertions(+), 45 deletions(-) diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts index 43478179f5b4..959d08fee1a9 100644 --- a/src/client/testing/testController/common/resultResolver.ts +++ b/src/client/testing/testController/common/resultResolver.ts @@ -4,7 +4,7 @@ import { CancellationToken, TestController, TestItem, Uri, TestRun, FileCoverageDetail } from 'vscode'; import { CoveragePayload, DiscoveredTestPayload, ExecutionTestPayload, ITestResultResolver } from './types'; import { TestProvider } from '../../types'; -import { traceVerbose } from '../../../logging'; +import { traceInfo } from '../../../logging'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { TestItemIndex } from './testItemIndex'; @@ -24,8 +24,6 @@ export class PythonResultResolver implements ITestResultResolver { private static executionHandler: TestExecutionHandler = new TestExecutionHandler(); private static coverageHandler: TestCoverageHandler = new TestCoverageHandler(); - public subTestStats: Map = new Map(); - public detailedCoverageMap = new Map(); constructor(testController: TestController, testProvider: TestProvider, private workspaceUri: Uri) { @@ -71,13 +69,14 @@ export class PythonResultResolver implements ITestResultResolver { public resolveExecution(payload: ExecutionTestPayload | CoveragePayload, runInstance: TestRun): void { if ('coverage' in payload) { // coverage data is sent once per connection - traceVerbose('Coverage data received.'); + traceInfo('Coverage data received, processing...'); this.detailedCoverageMap = PythonResultResolver.coverageHandler.processCoverage( payload as CoveragePayload, runInstance, ); + traceInfo('Coverage data processing complete.'); } else { - this.subTestStats = PythonResultResolver.executionHandler.processExecution( + PythonResultResolver.executionHandler.processExecution( payload as ExecutionTestPayload, runInstance, this.testItemIndex, diff --git a/src/client/testing/testController/common/testExecutionHandler.ts b/src/client/testing/testController/common/testExecutionHandler.ts index d524d9bb48ab..127e6980ae46 100644 --- a/src/client/testing/testController/common/testExecutionHandler.ts +++ b/src/client/testing/testController/common/testExecutionHandler.ts @@ -8,11 +8,6 @@ import { splitLines } from '../../../common/stringUtils'; import { splitTestNameWithRegex } from './utils'; import { clearAllChildren } from './testItemUtilities'; -export interface SubtestStats { - passed: number; - failed: number; -} - /** * Stateless handler for processing execution payloads and updating TestRun instances. * This handler is shared across all workspaces and contains no instance state. @@ -21,15 +16,13 @@ export class TestExecutionHandler { /** * Process execution payload and update test run * Pure function - no instance state used - * Returns subtest statistics for caller to manage */ public processExecution( payload: ExecutionTestPayload, runInstance: TestRun, testItemIndex: TestItemIndex, testController: TestController, - ): Map { - const subtestStats = new Map(); + ): void { const rawTestExecData = payload as ExecutionTestPayload; if (rawTestExecData !== undefined && rawTestExecData.result !== undefined) { @@ -37,11 +30,9 @@ export class TestExecutionHandler { const testItem = rawTestExecData.result[keyTemp]; // Delegate to specific outcome handlers - this.handleTestOutcome(keyTemp, testItem, runInstance, testItemIndex, testController, subtestStats); + this.handleTestOutcome(keyTemp, testItem, runInstance, testItemIndex, testController); } } - - return subtestStats; } /** @@ -53,7 +44,6 @@ export class TestExecutionHandler { runInstance: TestRun, testItemIndex: TestItemIndex, testController: TestController, - subtestStats: Map, ): void { if (testItem.outcome === 'error') { this.handleTestError(runId, testItem, runInstance, testItemIndex, testController); @@ -64,9 +54,9 @@ export class TestExecutionHandler { } else if (testItem.outcome === 'skipped') { this.handleTestSkipped(runId, runInstance, testItemIndex, testController); } else if (testItem.outcome === 'subtest-failure') { - this.handleSubtestFailure(runId, testItem, runInstance, testItemIndex, testController, subtestStats); + this.handleSubtestFailure(runId, testItem, runInstance, testItemIndex, testController); } else if (testItem.outcome === 'subtest-success') { - this.handleSubtestSuccess(runId, runInstance, testItemIndex, testController, subtestStats); + this.handleSubtestSuccess(runId, runInstance, testItemIndex, testController); } } @@ -168,17 +158,16 @@ export class TestExecutionHandler { runInstance: TestRun, testItemIndex: TestItemIndex, testController: TestController, - subtestStats: Map, ): void { const [parentTestCaseId, subtestId] = splitTestNameWithRegex(runId); const parentTestItem = testItemIndex.getTestItem(parentTestCaseId, testController); if (parentTestItem) { - const stats = subtestStats.get(parentTestCaseId); + const stats = testItemIndex.getSubtestStats(parentTestCaseId); if (stats) { stats.failed += 1; } else { - subtestStats.set(parentTestCaseId, { + testItemIndex.setSubtestStats(parentTestCaseId, { failed: 1, passed: 0, }); @@ -213,17 +202,16 @@ export class TestExecutionHandler { runInstance: TestRun, testItemIndex: TestItemIndex, testController: TestController, - subtestStats: Map, ): void { const [parentTestCaseId, subtestId] = splitTestNameWithRegex(runId); const parentTestItem = testItemIndex.getTestItem(parentTestCaseId, testController); if (parentTestItem) { - const stats = subtestStats.get(parentTestCaseId); + const stats = testItemIndex.getSubtestStats(parentTestCaseId); if (stats) { stats.passed += 1; } else { - subtestStats.set(parentTestCaseId, { failed: 0, passed: 1 }); + testItemIndex.setSubtestStats(parentTestCaseId, { failed: 0, passed: 1 }); clearAllChildren(parentTestItem); } diff --git a/src/client/testing/testController/common/testItemIndex.ts b/src/client/testing/testController/common/testItemIndex.ts index 25a5b8e55db7..448903eae7d5 100644 --- a/src/client/testing/testController/common/testItemIndex.ts +++ b/src/client/testing/testController/common/testItemIndex.ts @@ -5,6 +5,11 @@ import { TestController, TestItem } from 'vscode'; import { traceError, traceVerbose } from '../../../logging'; import { getTestCaseNodes } from './testItemUtilities'; +export interface SubtestStats { + passed: number; + failed: number; +} + /** * Maintains persistent ID mappings between Python test IDs and VS Code TestItems. * This is a stateful component that bridges discovery and execution phases. @@ -21,11 +26,13 @@ export class TestItemIndex { private runIdToTestItem: Map; private runIdToVSid: Map; private vsIdToRunId: Map; + private subtestStatsMap: Map; constructor() { this.runIdToTestItem = new Map(); this.runIdToVSid = new Map(); this.vsIdToRunId = new Map(); + this.subtestStatsMap = new Map(); } /** @@ -133,6 +140,21 @@ export class TestItemIndex { return testController.items.get(testItem.id) === testItem; } + /** + * Get subtest statistics for a parent test case + * Returns undefined if no stats exist yet for this parent + */ + public getSubtestStats(parentId: string): SubtestStats | undefined { + return this.subtestStatsMap.get(parentId); + } + + /** + * Set subtest statistics for a parent test case + */ + public setSubtestStats(parentId: string, stats: SubtestStats): void { + this.subtestStatsMap.set(parentId, stats); + } + /** * Remove all mappings * Called at the start of discovery to ensure clean state @@ -141,6 +163,7 @@ export class TestItemIndex { this.runIdToTestItem.clear(); this.runIdToVSid.clear(); this.vsIdToRunId.clear(); + this.subtestStatsMap.clear(); } /** diff --git a/src/test/testing/testController/common/testExecutionHandler.unit.test.ts b/src/test/testing/testController/common/testExecutionHandler.unit.test.ts index e0935d358dde..c6be4548c192 100644 --- a/src/test/testing/testController/common/testExecutionHandler.unit.test.ts +++ b/src/test/testing/testController/common/testExecutionHandler.unit.test.ts @@ -32,7 +32,7 @@ suite('TestExecutionHandler', () => { }); suite('processExecution', () => { - test('should return empty stats for empty payload', () => { + test('should process empty payload without errors', () => { const payload: ExecutionTestPayload = { cwd: '/foo/bar', status: 'success', @@ -40,31 +40,31 @@ suite('TestExecutionHandler', () => { error: '', }; - const stats = executionHandler.processExecution( + executionHandler.processExecution( payload, runInstanceMock.object, testItemIndexMock.object, testControllerMock.object, ); - assert.strictEqual(stats.size, 0); + // No errors should be thrown }); - test('should return empty stats for undefined result', () => { + test('should process undefined result without errors', () => { const payload: ExecutionTestPayload = { cwd: '/foo/bar', status: 'success', error: '', }; - const stats = executionHandler.processExecution( + executionHandler.processExecution( payload, runInstanceMock.object, testItemIndexMock.object, testControllerMock.object, ); - assert.strictEqual(stats.size, 0); + // No errors should be thrown }); test('should process multiple test results', () => { @@ -425,20 +425,30 @@ suite('TestExecutionHandler', () => { testItemIndexMock .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => undefined); + testItemIndexMock + .setup((x) => x.setSubtestStats('parentTest', typemoq.It.isAny())) + .returns(() => undefined); testControllerMock .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) .returns(() => mockSubtestItem); - const stats = executionHandler.processExecution( + executionHandler.processExecution( payload, runInstanceMock.object, testItemIndexMock.object, testControllerMock.object, ); - assert.strictEqual(stats.size, 1); - assert.strictEqual(stats.get('parentTest')?.failed, 1); - assert.strictEqual(stats.get('parentTest')?.passed, 0); + // Verify stats were set correctly + testItemIndexMock.verify( + (x) => + x.setSubtestStats( + 'parentTest', + typemoq.It.is((stats) => stats.failed === 1 && stats.passed === 0), + ), + typemoq.Times.once(), + ); runInstanceMock.verify((r) => r.started(mockSubtestItem), typemoq.Times.once()); runInstanceMock.verify((r) => r.failed(mockSubtestItem, typemoq.It.isAny()), typemoq.Times.once()); @@ -482,6 +492,12 @@ suite('TestExecutionHandler', () => { .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) .returns(() => mockParentItem); + // First subtest: no existing stats + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => undefined); + testItemIndexMock + .setup((x) => x.setSubtestStats('parentTest', typemoq.It.isAny())) + .returns(() => undefined); + // Return different items based on call order let callCount = 0; testControllerMock @@ -491,24 +507,30 @@ suite('TestExecutionHandler', () => { return callCount === 1 ? mockSubtest1 : mockSubtest2; }); - const stats1 = executionHandler.processExecution( + executionHandler.processExecution( payload1, runInstanceMock.object, testItemIndexMock.object, testControllerMock.object, ); - // Process second subtest with stats from first - const stats2 = executionHandler.processExecution( + // Second subtest: should have existing stats from first + testItemIndexMock.reset(); + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => ({ failed: 1, passed: 0 })); + + executionHandler.processExecution( payload2, runInstanceMock.object, testItemIndexMock.object, testControllerMock.object, ); - // Verify stats are separate for each call - assert.strictEqual(stats1.get('parentTest')?.failed, 1); - assert.strictEqual(stats2.get('parentTest')?.failed, 1); + // Verify the first subtest set initial stats + runInstanceMock.verify((r) => r.started(mockSubtest1), typemoq.Times.once()); + runInstanceMock.verify((r) => r.started(mockSubtest2), typemoq.Times.once()); }); test('should throw error when parent test item not found', () => { @@ -564,20 +586,30 @@ suite('TestExecutionHandler', () => { testItemIndexMock .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => undefined); + testItemIndexMock + .setup((x) => x.setSubtestStats('parentTest', typemoq.It.isAny())) + .returns(() => undefined); testControllerMock .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) .returns(() => mockSubtestItem); - const stats = executionHandler.processExecution( + executionHandler.processExecution( payload, runInstanceMock.object, testItemIndexMock.object, testControllerMock.object, ); - assert.strictEqual(stats.size, 1); - assert.strictEqual(stats.get('parentTest')?.passed, 1); - assert.strictEqual(stats.get('parentTest')?.failed, 0); + // Verify stats were set correctly + testItemIndexMock.verify( + (x) => + x.setSubtestStats( + 'parentTest', + typemoq.It.is((stats) => stats.passed === 1 && stats.failed === 0), + ), + typemoq.Times.once(), + ); runInstanceMock.verify((r) => r.started(mockSubtestItem), typemoq.Times.once()); runInstanceMock.verify((r) => r.passed(mockSubtestItem), typemoq.Times.once()); @@ -604,6 +636,10 @@ suite('TestExecutionHandler', () => { testItemIndexMock .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => undefined); + testItemIndexMock + .setup((x) => x.setSubtestStats('parentTest', typemoq.It.isAny())) + .returns(() => undefined); testControllerMock .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) .returns(() => mockSubtestItem); @@ -618,6 +654,252 @@ suite('TestExecutionHandler', () => { runInstanceMock.verify((r) => r.passed(mockSubtestItem), typemoq.Times.once()); }); }); + + suite('Comprehensive Subtest Scenarios', () => { + test('should handle mixed passing and failing subtests in sequence', () => { + // Simulates unittest with subtests like: test_even with i=0,1,2,3,4,5 + const mockSubtest0 = createMockTestItem('(i=0)', '(i=0)'); + const mockSubtest1 = createMockTestItem('(i=1)', '(i=1)'); + const mockSubtest2 = createMockTestItem('(i=2)', '(i=2)'); + const mockSubtest3 = createMockTestItem('(i=3)', '(i=3)'); + const mockSubtest4 = createMockTestItem('(i=4)', '(i=4)'); + const mockSubtest5 = createMockTestItem('(i=5)', '(i=5)'); + + const subtestItems = [mockSubtest0, mockSubtest1, mockSubtest2, mockSubtest3, mockSubtest4, mockSubtest5]; + + testItemIndexMock + .setup((x) => x.getTestItem('test_even', testControllerMock.object)) + .returns(() => mockParentItem); + + let subtestCallCount = 0; + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => subtestItems[subtestCallCount++]); + + // First subtest (i=0) - passes + testItemIndexMock.setup((x) => x.getSubtestStats('test_even')).returns(() => undefined); + testItemIndexMock.setup((x) => x.setSubtestStats('test_even', typemoq.It.isAny())).returns(() => undefined); + + const payload0: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'test_even (i=0)': { + test: 'test_even', + outcome: 'subtest-success', + message: '', + traceback: '', + subtest: '(i=0)', + }, + }, + error: '', + }; + + executionHandler.processExecution( + payload0, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Verify first subtest created stats + testItemIndexMock.verify( + (x) => + x.setSubtestStats( + 'test_even', + typemoq.It.is((stats) => stats.passed === 1 && stats.failed === 0), + ), + typemoq.Times.once(), + ); + + // Second subtest (i=1) - fails + testItemIndexMock.reset(); + testItemIndexMock + .setup((x) => x.getTestItem('test_even', testControllerMock.object)) + .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('test_even')).returns(() => ({ passed: 1, failed: 0 })); + + const payload1: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'test_even (i=1)': { + test: 'test_even', + outcome: 'subtest-failure', + message: '1 is not even', + traceback: 'AssertionError', + subtest: '(i=1)', + }, + }, + error: '', + }; + + executionHandler.processExecution( + payload1, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Third subtest (i=2) - passes + testItemIndexMock.reset(); + testItemIndexMock + .setup((x) => x.getTestItem('test_even', testControllerMock.object)) + .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('test_even')).returns(() => ({ passed: 1, failed: 1 })); + + const payload2: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'test_even (i=2)': { + test: 'test_even', + outcome: 'subtest-success', + message: '', + traceback: '', + subtest: '(i=2)', + }, + }, + error: '', + }; + + executionHandler.processExecution( + payload2, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Verify all subtests were started and had outcomes + runInstanceMock.verify((r) => r.started(mockSubtest0), typemoq.Times.once()); + runInstanceMock.verify((r) => r.passed(mockSubtest0), typemoq.Times.once()); + runInstanceMock.verify((r) => r.started(mockSubtest1), typemoq.Times.once()); + runInstanceMock.verify((r) => r.failed(mockSubtest1, typemoq.It.isAny()), typemoq.Times.once()); + runInstanceMock.verify((r) => r.started(mockSubtest2), typemoq.Times.once()); + runInstanceMock.verify((r) => r.passed(mockSubtest2), typemoq.Times.once()); + }); + + test('should persist stats across multiple processExecution calls', () => { + // Test that stats persist in TestItemIndex across multiple processExecution calls + const mockSubtest1 = createMockTestItem('subtest1', 'Subtest 1'); + const mockSubtest2 = createMockTestItem('subtest2', 'Subtest 2'); + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => undefined); + testItemIndexMock + .setup((x) => x.setSubtestStats('parentTest', typemoq.It.isAny())) + .returns(() => undefined); + + let callCount = 0; + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => (callCount++ === 0 ? mockSubtest1 : mockSubtest2)); + + const payload1: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest1)': { + test: 'parentTest', + outcome: 'subtest-success', + message: '', + traceback: '', + subtest: 'subtest1', + }, + }, + error: '', + }; + + // First call - no existing stats + executionHandler.processExecution( + payload1, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Simulate stats being stored in TestItemIndex + testItemIndexMock.reset(); + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => ({ passed: 1, failed: 0 })); + + const payload2: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest2)': { + test: 'parentTest', + outcome: 'subtest-failure', + message: 'Failed', + traceback: '', + subtest: 'subtest2', + }, + }, + error: '', + }; + + // Second call - existing stats should be retrieved and updated + executionHandler.processExecution( + payload2, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Verify getSubtestStats was called to retrieve existing stats + testItemIndexMock.verify((x) => x.getSubtestStats('parentTest'), typemoq.Times.once()); + + // Verify both subtests were processed + runInstanceMock.verify((r) => r.passed(mockSubtest1), typemoq.Times.once()); + runInstanceMock.verify((r) => r.failed(mockSubtest2, typemoq.It.isAny()), typemoq.Times.once()); + }); + + test('should clear children only on first subtest when no existing stats', () => { + // When first subtest arrives, children should be cleared + // Subsequent subtests should NOT clear children + const mockSubtest1 = createMockTestItem('subtest1', 'Subtest 1'); + + testItemIndexMock + .setup((x) => x.getTestItem('parentTest', testControllerMock.object)) + .returns(() => mockParentItem); + testItemIndexMock.setup((x) => x.getSubtestStats('parentTest')).returns(() => undefined); + testItemIndexMock + .setup((x) => x.setSubtestStats('parentTest', typemoq.It.isAny())) + .returns(() => undefined); + testControllerMock + .setup((t) => t.createTestItem(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => mockSubtest1); + + const payload: ExecutionTestPayload = { + cwd: '/foo/bar', + status: 'success', + result: { + 'parentTest (subtest1)': { + test: 'parentTest', + outcome: 'subtest-success', + message: '', + traceback: '', + subtest: 'subtest1', + }, + }, + error: '', + }; + + executionHandler.processExecution( + payload, + runInstanceMock.object, + testItemIndexMock.object, + testControllerMock.object, + ); + + // Verify setSubtestStats was called (which happens when creating new stats) + testItemIndexMock.verify((x) => x.setSubtestStats('parentTest', typemoq.It.isAny()), typemoq.Times.once()); + }); + }); }); function createMockTestItem(id: string, label: string): TestItem {