From 84df45cc892b5c12e0cd743804d05885cd312240 Mon Sep 17 00:00:00 2001 From: Eleanor Boyd Date: Fri, 11 Aug 2023 09:03:52 -0700 Subject: [PATCH] add std out to subprocess to stop overflow (#21758) Displays output to the user and ensures the subprocess doesn't run into buffer overflow. Bug was introduced with the switch to the execObservable in https://github.com/microsoft/vscode-python/pull/21667. Only occurs for large repos where output from pytest was large enough to reach buffer overflow. --- pythonFiles/tests/pytestadapter/test_execution.py | 1 - src/client/testing/testController/common/server.ts | 11 ++++++++++- .../testController/pytest/pytestDiscoveryAdapter.ts | 10 +++++++++- .../testController/pytest/pytestExecutionAdapter.ts | 9 +++++++++ src/test/mocks/helper.ts | 11 +++++++++++ src/test/mocks/mockChildProcess.ts | 7 ++++--- 6 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 src/test/mocks/helper.ts diff --git a/pythonFiles/tests/pytestadapter/test_execution.py b/pythonFiles/tests/pytestadapter/test_execution.py index 30f1a7e439d9..07354b01709b 100644 --- a/pythonFiles/tests/pytestadapter/test_execution.py +++ b/pythonFiles/tests/pytestadapter/test_execution.py @@ -1,6 +1,5 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. -import json import os import shutil diff --git a/src/client/testing/testController/common/server.ts b/src/client/testing/testController/common/server.ts index 564bd82f2ef6..661290bf4988 100644 --- a/src/client/testing/testController/common/server.ts +++ b/src/client/testing/testController/common/server.ts @@ -209,7 +209,16 @@ export class PythonTestServer implements ITestServer, Disposable { runInstance?.token.onCancellationRequested(() => { result?.proc?.kill(); }); - result?.proc?.on('close', () => { + + // Take all output from the subprocess and add it to the test output channel. This will be the pytest output. + // Displays output to user and ensure the subprocess doesn't run into buffer overflow. + result?.proc?.stdout?.on('data', (data) => { + spawnOptions?.outputChannel?.append(data); + }); + result?.proc?.stderr?.on('data', (data) => { + spawnOptions?.outputChannel?.append(data); + }); + result?.proc?.on('exit', () => { traceLog('Exec server closed.', uuid); deferred.resolve({ stdout: '', stderr: '' }); callback?.(); diff --git a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts index dbb92b8370ba..0c91e3c94bdc 100644 --- a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts +++ b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts @@ -86,7 +86,15 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs); const result = execService?.execObservable(execArgs, spawnOptions); - result?.proc?.on('close', () => { + // Take all output from the subprocess and add it to the test output channel. This will be the pytest output. + // Displays output to user and ensure the subprocess doesn't run into buffer overflow. + result?.proc?.stdout?.on('data', (data) => { + spawnOptions.outputChannel?.append(data); + }); + result?.proc?.stderr?.on('data', (data) => { + spawnOptions.outputChannel?.append(data); + }); + result?.proc?.on('exit', () => { deferredExec.resolve({ stdout: '', stderr: '' }); deferred.resolve(); }); diff --git a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts index e89e14bd8d84..653de36a3e23 100644 --- a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts +++ b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts @@ -166,6 +166,15 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { result?.proc?.kill(); }); + // Take all output from the subprocess and add it to the test output channel. This will be the pytest output. + // Displays output to user and ensure the subprocess doesn't run into buffer overflow. + result?.proc?.stdout?.on('data', (data) => { + this.outputChannel?.append(data); + }); + result?.proc?.stderr?.on('data', (data) => { + this.outputChannel?.append(data); + }); + result?.proc?.on('close', () => { deferredExec.resolve({ stdout: '', stderr: '' }); deferred.resolve(); diff --git a/src/test/mocks/helper.ts b/src/test/mocks/helper.ts new file mode 100644 index 000000000000..24d7a8290b18 --- /dev/null +++ b/src/test/mocks/helper.ts @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { Readable } from 'stream'; + +export class FakeReadableStream extends Readable { + _read(_size: unknown): void | null { + // custom reading logic here + this.push(null); // end the stream + } +} diff --git a/src/test/mocks/mockChildProcess.ts b/src/test/mocks/mockChildProcess.ts index c038c0f845ab..a46d66d79ca0 100644 --- a/src/test/mocks/mockChildProcess.ts +++ b/src/test/mocks/mockChildProcess.ts @@ -1,8 +1,9 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ /* eslint-disable @typescript-eslint/no-explicit-any */ import { Serializable, SendHandle, MessageOptions } from 'child_process'; -import { Writable, Readable, Pipe } from 'stream'; import { EventEmitter } from 'node:events'; +import { Writable, Readable, Pipe } from 'stream'; +import { FakeReadableStream } from './helper'; export class MockChildProcess extends EventEmitter { constructor(spawnfile: string, spawnargs: string[]) { @@ -10,8 +11,8 @@ export class MockChildProcess extends EventEmitter { this.spawnfile = spawnfile; this.spawnargs = spawnargs; this.stdin = new Writable(); - this.stdout = new Readable(); - this.stderr = null; + this.stdout = new FakeReadableStream(); + this.stderr = new FakeReadableStream(); this.channel = null; this.stdio = [this.stdin, this.stdout, this.stdout, this.stderr, null]; this.killed = false;