handle subprocess segfaults for testAdapters (microsoft#21963)

closes: microsoft#21662 Not only does this make sure segfaults are correct for unittest but also for pytest.
karthiknadig · Sep 12, 2023 · df0b493 · df0b493
1 parent 7aa6660
commit df0b493
Show file tree

Hide file tree

Showing 9 changed files with 191 additions and 22 deletions.
diff --git a/src/client/testing/testController/common/server.ts b/src/client/testing/testController/common/server.ts
@@ -15,7 +15,7 @@ import { traceError, traceInfo, traceLog } from '../../../logging';
 import { DataReceivedEvent, ITestServer, TestCommandOptions } from './types';
 import { ITestDebugLauncher, LaunchOptions } from '../../common/types';
 import { UNITTEST_PROVIDER } from '../../common/constants';
-import { jsonRPCHeaders, jsonRPCContent, JSONRPC_UUID_HEADER } from './utils';
+import { jsonRPCHeaders, jsonRPCContent, JSONRPC_UUID_HEADER, createExecutionErrorPayload } from './utils';
 import { createDeferred } from '../../../common/utils/async';
 
 export class PythonTestServer implements ITestServer, Disposable {
@@ -133,6 +133,10 @@ export class PythonTestServer implements ITestServer, Disposable {
         return this._onDiscoveryDataReceived.event;
     }
 
+    public triggerRunDataReceivedEvent(payload: DataReceivedEvent): void {
+        this._onRunDataReceived.fire(payload);
+    }
+
     public dispose(): void {
         this.server.close();
         this._onDataReceived.dispose();
@@ -146,6 +150,7 @@ export class PythonTestServer implements ITestServer, Disposable {
         options: TestCommandOptions,
         runTestIdPort?: string,
         runInstance?: TestRun,
+        testIds?: string[],
         callback?: () => void,
     ): Promise<void> {
         const { uuid } = options;
@@ -218,8 +223,15 @@ export class PythonTestServer implements ITestServer, Disposable {
                 result?.proc?.stderr?.on('data', (data) => {
                     spawnOptions?.outputChannel?.append(data.toString());
                 });
-                result?.proc?.on('exit', () => {
-                    traceLog('Exec server closed.', uuid);
+                result?.proc?.on('exit', (code, signal) => {
+                    // if the child has testIds then this is a run request
+                    if (code !== 0 && testIds) {
+                        // if the child process exited with a non-zero exit code, then we need to send the error payload.
+                        this._onRunDataReceived.fire({
+                            uuid,
+                            data: JSON.stringify(createExecutionErrorPayload(code, signal, testIds, options.cwd)),
+                        });
+                    }
                     deferred.resolve({ stdout: '', stderr: '' });
                     callback?.();
                 });

diff --git a/src/client/testing/testController/common/types.ts b/src/client/testing/testController/common/types.ts
@@ -178,12 +178,14 @@ export interface ITestServer {
         options: TestCommandOptions,
         runTestIdsPort?: string,
         runInstance?: TestRun,
+        testIds?: string[],
         callback?: () => void,
     ): Promise<void>;
     serverReady(): Promise<void>;
     getPort(): number;
     createUUID(cwd: string): string;
     deleteUUID(uuid: string): void;
+    triggerRunDataReceivedEvent(data: DataReceivedEvent): void;
 }
 export interface ITestResultResolver {
     runIdToVSid: Map<string, string>;

diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts
@@ -9,7 +9,7 @@ import { EnableTestAdapterRewrite } from '../../../common/experiments/groups';
 import { IExperimentService } from '../../../common/types';
 import { IServiceContainer } from '../../../ioc/types';
 import { DebugTestTag, ErrorTestItemOptions, RunTestTag } from './testItemUtilities';
-import { DiscoveredTestItem, DiscoveredTestNode, ITestResultResolver } from './types';
+import { DiscoveredTestItem, DiscoveredTestNode, ExecutionTestPayload, ITestResultResolver } from './types';
 
 export function fixLogLines(content: string): string {
     const lines = content.split(/\r?\n/g);
@@ -188,3 +188,27 @@ export function populateTestTree(
 function isTestItem(test: DiscoveredTestNode | DiscoveredTestItem): test is DiscoveredTestItem {
     return test.type_ === 'test';
 }
+
+export function createExecutionErrorPayload(
+    code: number | null,
+    signal: NodeJS.Signals | null,
+    testIds: string[],
+    cwd: string,
+): ExecutionTestPayload {
+    const etp: ExecutionTestPayload = {
+        cwd,
+        status: 'error',
+        error: 'Test run failed, the python test process was terminated before it could exit on its own.',
+        result: {},
+    };
+    // add error result for each attempted test.
+    for (let i = 0; i < testIds.length; i = i + 1) {
+        const test = testIds[i];
+        etp.result![test] = {
+            test,
+            outcome: 'error',
+            message: ` \n The python test process was terminated before it could exit on its own, the process errored with: Code: ${code}, Signal: ${signal}`,
+        };
+    }
+    return etp;
+}
diff --git a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts
@@ -5,7 +5,7 @@ import { TestRun, Uri } from 'vscode';
 import * as path from 'path';
 import { IConfigurationService, ITestOutputChannel } from '../../../common/types';
 import { createDeferred } from '../../../common/utils/async';
-import { traceError, traceInfo, traceLog, traceVerbose } from '../../../logging';
+import { traceError, traceInfo, traceVerbose } from '../../../logging';
 import {
     DataReceivedEvent,
     ExecutionTestPayload,
@@ -130,7 +130,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
             if (debugBool && !testArgs.some((a) => a.startsWith('--capture') || a === '-s')) {
                 testArgs.push('--capture', 'no');
             }
-            traceLog(`Running PYTEST execution for the following test ids: ${testIds}`);
 
             const pytestRunTestIdsPort = await utils.startTestIdServer(testIds);
             if (spawnOptions.extraVariables)
@@ -175,7 +174,15 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
                     this.outputChannel?.append(data.toString());
                 });
 
-                result?.proc?.on('exit', () => {
+                result?.proc?.on('exit', (code, signal) => {
+                    // if the child has testIds then this is a run request
+                    if (code !== 0 && testIds) {
+                        // if the child process exited with a non-zero exit code, then we need to send the error payload.
+                        this.testServer.triggerRunDataReceivedEvent({
+                            uuid,
+                            data: JSON.stringify(utils.createExecutionErrorPayload(code, signal, testIds, cwd)),
+                        });
+                    }
                     deferredExec.resolve({ stdout: '', stderr: '' });
                     deferred.resolve();
                     disposeDataReceiver?.(this.testServer);

diff --git a/src/client/testing/testController/unittest/testDiscoveryAdapter.ts b/src/client/testing/testController/unittest/testDiscoveryAdapter.ts
@@ -64,7 +64,7 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
     }
 
     private async callSendCommand(options: TestCommandOptions, callback: () => void): Promise<DiscoveredTestPayload> {
-        await this.testServer.sendCommand(options, undefined, undefined, callback);
+        await this.testServer.sendCommand(options, undefined, undefined, [], callback);
         const discoveryPayload: DiscoveredTestPayload = { cwd: '', status: 'success' };
         return discoveryPayload;
     }

diff --git a/src/client/testing/testController/unittest/testExecutionAdapter.ts b/src/client/testing/testController/unittest/testExecutionAdapter.ts
@@ -83,7 +83,7 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
 
         const runTestIdsPort = await startTestIdServer(testIds);
 
-        await this.testServer.sendCommand(options, runTestIdsPort.toString(), runInstance, () => {
+        await this.testServer.sendCommand(options, runTestIdsPort.toString(), runInstance, testIds, () => {
             deferred.resolve();
             disposeDataReceiver?.(this.testServer);
         });

diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts
@@ -13,7 +13,7 @@ import { ITestDebugLauncher } from '../../../client/testing/common/types';
 import { IConfigurationService, ITestOutputChannel } from '../../../client/common/types';
 import { IServiceContainer } from '../../../client/ioc/types';
 import { EXTENSION_ROOT_DIR_FOR_TESTS, initialize } from '../../initialize';
-import { traceError, traceLog } from '../../../client/logging';
+import { traceLog } from '../../../client/logging';
 import { PytestTestExecutionAdapter } from '../../../client/testing/testController/pytest/pytestExecutionAdapter';
 import { UnittestTestDiscoveryAdapter } from '../../../client/testing/testController/unittest/testDiscoveryAdapter';
 import { UnittestTestExecutionAdapter } from '../../../client/testing/testController/unittest/testExecutionAdapter';
@@ -39,6 +39,12 @@ suite('End to End Tests: test adapters', () => {
         'testTestingRootWkspc',
         'largeWorkspace',
     );
+    const rootPathErrorWorkspace = path.join(
+        EXTENSION_ROOT_DIR_FOR_TESTS,
+        'src',
+        'testTestingRootWkspc',
+        'errorWorkspace',
+    );
     suiteSetup(async () => {
         serviceContainer = (await initialize()).serviceContainer;
     });
@@ -86,10 +92,21 @@ suite('End to End Tests: test adapters', () => {
 
         await discoveryAdapter.discoverTests(workspaceUri).finally(() => {
             // verification after discovery is complete
-            resultResolver.verify(
-                (x) => x.resolveDiscovery(typeMoq.It.isAny(), typeMoq.It.isAny()),
-                typeMoq.Times.once(),
-            );
+            // resultResolver.verify(
+            //     (x) => x.resolveDiscovery(typeMoq.It.isAny(), typeMoq.It.isAny()),
+            //     typeMoq.Times.once(),
+            // );
+
+            // 1. Check the status is "success"
+            assert.strictEqual(actualData.status, 'success', "Expected status to be 'success'");
+            // 2. Confirm no errors
+            assert.strictEqual(actualData.error, undefined, "Expected no errors in 'error' field");
+            // 3. Confirm tests are found
+            assert.ok(actualData.tests, 'Expected tests to be present');
+        });
+
+        await discoveryAdapter.discoverTests(Uri.parse(rootPathErrorWorkspace)).finally(() => {
+            // verification after discovery is complete
 
             // 1. Check the status is "success"
             assert.strictEqual(actualData.status, 'success', "Expected status to be 'success'");
@@ -191,7 +208,6 @@ suite('End to End Tests: test adapters', () => {
         resultResolver
             .setup((x) => x.resolveDiscovery(typeMoq.It.isAny(), typeMoq.It.isAny()))
             .returns((data) => {
-                traceLog(`resolveDiscovery ${data}`);
                 actualData = data;
                 return Promise.resolve();
             });
@@ -231,7 +247,6 @@ suite('End to End Tests: test adapters', () => {
         resultResolver
             .setup((x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()))
             .returns((data) => {
-                traceLog(`resolveExecution ${data}`);
                 actualData = data;
                 return Promise.resolve();
             });
@@ -263,7 +278,6 @@ suite('End to End Tests: test adapters', () => {
                     (x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()),
                     typeMoq.Times.once(),
                 );
-
                 // 1. Check the status is "success"
                 assert.strictEqual(actualData.status, 'success', "Expected status to be 'success'");
                 // 2. Confirm tests are found
@@ -275,7 +289,6 @@ suite('End to End Tests: test adapters', () => {
         resultResolver
             .setup((x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()))
             .returns((data) => {
-                traceError(`resolveExecution ${data}`);
                 traceLog(`resolveExecution ${data}`);
                 // do the following asserts for each time resolveExecution is called, should be called once per test.
                 // 1. Check the status, can be subtest success or failure
@@ -328,7 +341,6 @@ suite('End to End Tests: test adapters', () => {
         resultResolver
             .setup((x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()))
             .returns((data) => {
-                traceLog(`resolveExecution ${data}`);
                 actualData = data;
                 return Promise.resolve();
             });
@@ -366,7 +378,6 @@ suite('End to End Tests: test adapters', () => {
                     (x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()),
                     typeMoq.Times.once(),
                 );
-
                 // 1. Check the status is "success"
                 assert.strictEqual(actualData.status, 'success', "Expected status to be 'success'");
                 // 2. Confirm no errors
@@ -379,7 +390,6 @@ suite('End to End Tests: test adapters', () => {
         resultResolver
             .setup((x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()))
             .returns((data) => {
-                traceLog(`resolveExecution ${data}`);
                 // do the following asserts for each time resolveExecution is called, should be called once per test.
                 // 1. Check the status is "success"
                 assert.strictEqual(data.status, 'success', "Expected status to be 'success'");
@@ -424,4 +434,100 @@ suite('End to End Tests: test adapters', () => {
             );
         });
     });
+    test('unittest execution adapter seg fault error handling', async () => {
+        const testId = `test_seg_fault.TestSegmentationFault.test_segfault`;
+        const testIds: string[] = [testId];
+        resultResolver
+            .setup((x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()))
+            .returns((data) => {
+                // do the following asserts for each time resolveExecution is called, should be called once per test.
+                // 1. Check the status is "success"
+                assert.strictEqual(data.status, 'error', "Expected status to be 'error'");
+                // 2. Confirm no errors
+                assert.ok(data.error, "Expected errors in 'error' field");
+                // 3. Confirm tests are found
+                assert.ok(data.result, 'Expected results to be present');
+                // 4. make sure the testID is found in the results
+                assert.notDeepEqual(
+                    JSON.stringify(data).search('test_seg_fault.TestSegmentationFault.test_segfault'),
+                    -1,
+                    'Expected testId to be present',
+                );
+                return Promise.resolve();
+            });
+
+        // set workspace to test workspace folder
+        workspaceUri = Uri.parse(rootPathErrorWorkspace);
+
+        // run pytest execution
+        const executionAdapter = new UnittestTestExecutionAdapter(
+            pythonTestServer,
+            configService,
+            testOutputChannel,
+            resultResolver.object,
+        );
+        const testRun = typeMoq.Mock.ofType<TestRun>();
+        testRun
+            .setup((t) => t.token)
+            .returns(
+                () =>
+                    ({
+                        onCancellationRequested: () => undefined,
+                    } as any),
+            );
+        await executionAdapter.runTests(workspaceUri, testIds, false, testRun.object).finally(() => {
+            resultResolver.verify(
+                (x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()),
+                typeMoq.Times.exactly(1),
+            );
+        });
+    });
+    test('pytest execution adapter seg fault error handling', async () => {
+        const testId = `${rootPathErrorWorkspace}/test_seg_fault.py::TestSegmentationFault::test_segfault`;
+        const testIds: string[] = [testId];
+        resultResolver
+            .setup((x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()))
+            .returns((data) => {
+                // do the following asserts for each time resolveExecution is called, should be called once per test.
+                // 1. Check the status is "success"
+                assert.strictEqual(data.status, 'error', "Expected status to be 'error'");
+                // 2. Confirm no errors
+                assert.ok(data.error, "Expected errors in 'error' field");
+                // 3. Confirm tests are found
+                assert.ok(data.result, 'Expected results to be present');
+                // 4. make sure the testID is found in the results
+                assert.notDeepEqual(
+                    JSON.stringify(data).search('test_seg_fault.py::TestSegmentationFault::test_segfault'),
+                    -1,
+                    'Expected testId to be present',
+                );
+                return Promise.resolve();
+            });
+
+        // set workspace to test workspace folder
+        workspaceUri = Uri.parse(rootPathErrorWorkspace);
+
+        // run pytest execution
+        const executionAdapter = new PytestTestExecutionAdapter(
+            pythonTestServer,
+            configService,
+            testOutputChannel,
+            resultResolver.object,
+        );
+        const testRun = typeMoq.Mock.ofType<TestRun>();
+        testRun
+            .setup((t) => t.token)
+            .returns(
+                () =>
+                    ({
+                        onCancellationRequested: () => undefined,
+                    } as any),
+            );
+        await executionAdapter.runTests(workspaceUri, testIds, false, testRun.object, pythonExecFactory).finally(() => {
+            resultResolver.verify(
+                (x) => x.resolveExecution(typeMoq.It.isAny(), typeMoq.It.isAny()),
+                typeMoq.Times.exactly(1),
+            );
+        });
+    });
 });
diff --git a/src/testTestingRootWkspc/errorWorkspace/test_seg_fault.py b/src/testTestingRootWkspc/errorWorkspace/test_seg_fault.py
@@ -0,0 +1,18 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+import unittest
+import ctypes
+
+
+class TestSegmentationFault(unittest.TestCase):
+    def cause_segfault(self):
+        ctypes.string_at(0)  # Dereference a NULL pointer
+
+    def test_segfault(self):
+        assert True
+        self.cause_segfault()
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/src/testTestingRootWkspc/largeWorkspace/test_parameterized_subtest.py b/src/testTestingRootWkspc/largeWorkspace/test_parameterized_subtest.py
@@ -6,7 +6,7 @@
 
 @pytest.mark.parametrize("num", range(0, 200))
 def test_odd_even(num):
-    return num % 2 == 0
+    assert num % 2 == 0
 
 
 class NumbersTest(unittest.TestCase):