diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index dc1872012c0f..a791e563ef5d 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -89,6 +89,8 @@ export async function startRunResultNamedPipe( traceVerbose('Starting Test Result named pipe'); const pipeName: string = generateRandomPipeName('python-test-results'); + // `cancellationToken` only cancels pipe creation; disposal is driven by + // `reader.onClose` so buffered results are not dropped on cancel. const reader = await createReaderPipe(pipeName, cancellationToken); traceVerbose(`Test Results named pipe ${pipeName} connected`); let disposables: Disposable[] = []; @@ -99,14 +101,6 @@ export async function startRunResultNamedPipe( deferredTillServerClose.resolve(); }); - if (cancellationToken) { - disposables.push( - cancellationToken?.onCancellationRequested(() => { - traceLog(`Test Result named pipe ${pipeName} cancelled`); - disposable.dispose(); - }), - ); - } disposables.push( reader, reader.listen((data: Message) => { @@ -115,9 +109,7 @@ export async function startRunResultNamedPipe( dataReceivedCallback((data as ExecutionResultMessage).params as ExecutionTestPayload); }), reader.onClose(() => { - // this is called once the server close, once per run instance traceVerbose(`Test Result named pipe ${pipeName} closed. Disposing of listener/s.`); - // dispose of all data listeners and cancelation listeners disposable.dispose(); }), reader.onError((error) => { diff --git a/src/client/testing/testController/unittest/testExecutionAdapter.ts b/src/client/testing/testController/unittest/testExecutionAdapter.ts index c7d21b768c5b..8acc4c0dfed8 100644 --- a/src/client/testing/testController/unittest/testExecutionAdapter.ts +++ b/src/client/testing/testController/unittest/testExecutionAdapter.ts @@ -70,9 +70,10 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter { cSource.token, // token to cancel ); runInstance.token.onCancellationRequested(() => { - console.log(`Test run cancelled, resolving 'till TillAllServerClose' deferred for ${uri.fsPath}.`); - // if canceled, stop listening for results - deferredTillServerClose.resolve(); + console.log(`Test run cancelled for ${uri.fsPath}; waiting for result pipe to drain.`); + // Don't resolve the deferred here: the pipe must drain first. + // `reader.onClose` in `startRunResultNamedPipe` will resolve it + // once the subprocess closes its end of the pipe. }); try { await this.runTestsNew( diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index 3cba6fb697a5..7c7055a752fd 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -2,12 +2,21 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as fs from 'fs'; import * as path from 'path'; -import { CancellationToken, TestController, TestItem, Uri, Range, Position } from 'vscode'; -import { writeTestIdsFile, populateTestTree } from '../../../client/testing/testController/common/utils'; +import { CancellationToken, CancellationTokenSource, TestController, TestItem, Uri, Range, Position } from 'vscode'; +import { Emitter, Event, MessageReader, PartialMessageInfo, Disposable as RpcDisposable, DataCallback } from 'vscode-jsonrpc'; +import { Message } from 'vscode-jsonrpc'; +import { + writeTestIdsFile, + populateTestTree, + startRunResultNamedPipe, +} from '../../../client/testing/testController/common/utils'; +import { createDeferred, Deferred } from '../../../client/common/utils/async'; +import * as namedPipes from '../../../client/common/pipes/namedPipes'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { DiscoveredTestNode, DiscoveredTestItem, + ExecutionTestPayload, ITestResultResolver, } from '../../../client/testing/testController/common/types'; import { RunTestTag, DebugTestTag } from '../../../client/testing/testController/common/testItemUtilities'; @@ -752,3 +761,166 @@ suite('populateTestTree tests', () => { assert.deepStrictEqual(mockTestItem2.range, new Range(new Position(6, 0), new Position(7, 0))); }); }); + +suite('startRunResultNamedPipe drain-on-cancel tests', () => { + let sandbox: sinon.SinonSandbox; + let createReaderPipeStub: sinon.SinonStub; + + // Minimal `MessageReader` fake exposing only what `startRunResultNamedPipe` uses. + class FakeMessageReader implements MessageReader { + private _onClose = new Emitter(); + + private _onError = new Emitter(); + + private _onPartialMessage = new Emitter(); + + private _callback: DataCallback | undefined; + + public disposed = false; + + public onError: Event = this._onError.event; + + public onClose: Event = this._onClose.event; + + public onPartialMessage: Event = this._onPartialMessage.event; + + public listen(callback: DataCallback): RpcDisposable { + this._callback = callback; + return { dispose: () => { this._callback = undefined; } }; + } + + public dispose(): void { + this.disposed = true; + this._onClose.dispose(); + this._onError.dispose(); + this._onPartialMessage.dispose(); + } + + // Test helpers. + public emit(message: Message): void { + this._callback?.(message); + } + + public hasListener(): boolean { + return this._callback !== undefined; + } + + public fireClose(): void { + this._onClose.fire(); + } + } + + setup(() => { + sandbox = sinon.createSandbox(); + }); + + teardown(() => { + sandbox.restore(); + }); + + function makeMessage(payload: Partial): Message { + return ({ jsonrpc: '2.0', params: payload } as unknown) as Message; + } + + test('cancellation alone does NOT resolve deferredTillServerClose and does NOT detach the listener (drain not interrupted)', async () => { + const reader = new FakeMessageReader(); + createReaderPipeStub = sandbox.stub(namedPipes, 'createReaderPipe').resolves(reader); + + const received: ExecutionTestPayload[] = []; + const deferredTillServerClose: Deferred = createDeferred(); + const cancelSource = new CancellationTokenSource(); + + await startRunResultNamedPipe( + (payload) => received.push(payload), + deferredTillServerClose, + cancelSource.token, + ); + + assert.ok(createReaderPipeStub.calledOnce, 'createReaderPipe should be called once'); + assert.ok(reader.hasListener(), 'reader should have a listener registered before cancel'); + + // Trigger cancellation. + cancelSource.cancel(); + + // Yield to let any synchronous-then-microtask handlers run. + await new Promise((r) => setImmediate(r)); + + assert.strictEqual( + reader.disposed, + false, + 'reader must NOT be disposed by cancellation alone (otherwise buffered data would be lost)', + ); + assert.ok(reader.hasListener(), 'data listener must remain attached after cancel so the drain can continue'); + assert.strictEqual( + (deferredTillServerClose as Deferred).completed, + false, + 'deferredTillServerClose must NOT resolve on cancellation; it should only resolve when the pipe closes', + ); + + cancelSource.dispose(); + }); + + test('data emitted after cancellation is still delivered to the callback (drain works)', async () => { + const reader = new FakeMessageReader(); + sandbox.stub(namedPipes, 'createReaderPipe').resolves(reader); + + const received: ExecutionTestPayload[] = []; + const deferredTillServerClose: Deferred = createDeferred(); + const cancelSource = new CancellationTokenSource(); + + await startRunResultNamedPipe( + (payload) => received.push(payload), + deferredTillServerClose, + cancelSource.token, + ); + + // Simulate the debug-path race: cancel fires while results are still buffered. + cancelSource.cancel(); + await new Promise((r) => setImmediate(r)); + + // Buffered messages arrive after cancellation. + reader.emit(makeMessage({ cwd: 'a' })); + reader.emit(makeMessage({ cwd: 'b' })); + + assert.strictEqual(received.length, 2, 'messages emitted after cancel must still reach the callback'); + assert.deepStrictEqual( + received.map((p) => p.cwd), + ['a', 'b'], + 'all buffered results delivered in order', + ); + + // Subprocess closes its end of the pipe -> onClose fires -> dispose. + reader.fireClose(); + await deferredTillServerClose.promise; + + assert.strictEqual(reader.disposed, true, 'reader disposed via onClose path'); + + cancelSource.dispose(); + }); + + test('reader.onClose resolves deferredTillServerClose and disposes the reader (natural completion path, no cancellation)', async () => { + const reader = new FakeMessageReader(); + sandbox.stub(namedPipes, 'createReaderPipe').resolves(reader); + + const deferredTillServerClose: Deferred = createDeferred(); + + await startRunResultNamedPipe( + () => { + /* no-op */ + }, + deferredTillServerClose, + undefined, + ); + + assert.strictEqual( + (deferredTillServerClose as Deferred).completed, + false, + 'deferred unresolved before close', + ); + + reader.fireClose(); + await deferredTillServerClose.promise; + + assert.strictEqual(reader.disposed, true, 'reader disposed when onClose fires'); + }); +});