Ensure subtest run update parent results (#17578)

karthiknadig · web-flow · commit ac9752a01692 · 2021-10-11T15:08:59.000-07:00
* Support for subtest run results.

* Add news

* Ensure engine version supports testtags

* Don't use proposed constructor
diff --git a/news/2 Fixes/17561.md b/news/2 Fixes/17561.md
@@ -0,0 +1 @@
+Ensures test node is updated when `unittest` sub-tests are used.
diff --git a/package.json b/package.json
@@ -37,7 +37,7 @@
         "theme": "dark"
     },
     "engines": {
-        "vscode": "^1.59.0"
+        "vscode": "^1.61.0"
     },
     "keywords": [
         "python",
diff --git a/pythonFiles/visualstudio_py_testlauncher.py b/pythonFiles/visualstudio_py_testlauncher.py
@@ -168,7 +168,13 @@ def addUnexpectedSuccess(self, test):
         super(VsTestResult, self).addUnexpectedSuccess(test)
         self.sendResult(test, "passed-unexpected")
 
-    def sendResult(self, test, outcome, trace=None):
+    def addSubTest(self, test, subtest, err):
+        super(VsTestResult, self).addSubTest(test, subtest, err)
+        self.sendResult(
+            test, "subtest-passed" if err is None else "subtest-failed", err, subtest
+        )
+
+    def sendResult(self, test, outcome, trace=None, subtest=None):
         if _channel is not None:
             tb = None
             message = None
@@ -179,13 +185,16 @@ def sendResult(self, test, outcome, trace=None):
                 formatted = formatted[1:]
                 tb = "".join(formatted)
                 message = str(trace[1])
-            _channel.send_event(
-                name="result",
-                outcome=outcome,
-                traceback=tb,
-                message=message,
-                test=test.id(),
-            )
+
+            result = {
+                "outcome": outcome,
+                "traceback": tb,
+                "message": message,
+                "test": test.id(),
+            }
+            if subtest is not None:
+                result["subtest"] = subtest.id()
+            _channel.send_event("result", **result)
 
 
 def stopTests():
diff --git a/src/client/testing/testController/common/testItemUtilities.ts b/src/client/testing/testController/common/testItemUtilities.ts
@@ -25,6 +25,10 @@ import {
     TestDataKinds,
 } from './types';
 
+// Todo: Use `TestTag` when the proposed API gets into stable.
+export const RunTestTag = { id: 'python-run' };
+export const DebugTestTag = { id: 'python-debug' };
+
 export function removeItemByIdFromChildren(
     idToRawData: Map<string, TestData>,
     item: TestItem,
@@ -43,6 +47,7 @@ export function createErrorTestItem(
     const testItem = testController.createTestItem(options.id, options.label);
     testItem.canResolveChildren = false;
     testItem.error = options.error;
+    testItem.tags = [RunTestTag, DebugTestTag];
     return testItem;
 }
 
@@ -58,6 +63,7 @@ export function createWorkspaceRootTestItem(
         rawId: options.rawId ?? options.id,
         kind: TestDataKinds.Workspace,
     });
+    testItem.tags = [RunTestTag, DebugTestTag];
     return testItem;
 }
 
@@ -125,6 +131,7 @@ function createFolderOrFileTestItem(
         kind: TestDataKinds.FolderOrFile,
         parentId,
     });
+    testItem.tags = [RunTestTag, DebugTestTag];
     return testItem;
 }
 
@@ -151,6 +158,7 @@ function updateFolderOrFileTestItem(
         kind: TestDataKinds.FolderOrFile,
         parentId,
     });
+    item.tags = [RunTestTag, DebugTestTag];
 }
 
 function createCollectionTestItem(
@@ -184,6 +192,7 @@ function createCollectionTestItem(
         kind: TestDataKinds.Collection,
         parentId,
     });
+    testItem.tags = [RunTestTag, DebugTestTag];
     return testItem;
 }
 
@@ -213,6 +222,7 @@ function updateCollectionTestItem(
         kind: TestDataKinds.Collection,
         parentId,
     });
+    item.tags = [RunTestTag, DebugTestTag];
 }
 
 function createTestCaseItem(
@@ -249,6 +259,7 @@ function createTestCaseItem(
         kind: TestDataKinds.Case,
         parentId,
     });
+    testItem.tags = [RunTestTag, DebugTestTag];
     return testItem;
 }
 
@@ -279,6 +290,7 @@ function updateTestCaseItem(
         kind: TestDataKinds.Case,
         parentId,
     });
+    item.tags = [RunTestTag, DebugTestTag];
 }
 
 function updateTestItemFromRawDataInternal(
@@ -475,7 +487,7 @@ export function getUri(node: TestItem): Uri | undefined {
 }
 
 export function getTestCaseNodes(testNode: TestItem, collection: TestItem[] = []): TestItem[] {
-    if (!testNode.canResolveChildren) {
+    if (!testNode.canResolveChildren && testNode.tags.length > 0) {
         collection.push(testNode);
     }
 
@@ -551,3 +563,9 @@ export function checkForFailedTests(resultMap: Map<string, TestResultState>): bo
         ) !== undefined
     );
 }
+
+export function clearAllChildren(testNode: TestItem): void {
+    const ids: string[] = [];
+    testNode.children.forEach((c) => ids.push(c.id));
+    ids.forEach(testNode.children.delete);
+}
diff --git a/src/client/testing/testController/common/types.ts b/src/client/testing/testController/common/types.ts
@@ -61,12 +61,22 @@ export const ITestFrameworkController = Symbol('ITestFrameworkController');
 export interface ITestFrameworkController {
     resolveChildren(testController: TestController, item: TestItem, token?: CancellationToken): Promise<void>;
     refreshTestData(testController: TestController, resource?: Uri, token?: CancellationToken): Promise<void>;
-    runTests(testRun: ITestRun, workspace: WorkspaceFolder, token: CancellationToken): Promise<void>;
+    runTests(
+        testRun: ITestRun,
+        workspace: WorkspaceFolder,
+        token: CancellationToken,
+        testController?: TestController,
+    ): Promise<void>;
 }
 
 export const ITestsRunner = Symbol('ITestsRunner');
 export interface ITestsRunner {
-    runTests(testRun: ITestRun, options: TestRunOptions, idToRawData: Map<string, TestData>): Promise<void>;
+    runTests(
+        testRun: ITestRun,
+        options: TestRunOptions,
+        idToRawData: Map<string, TestData>,
+        testController?: TestController,
+    ): Promise<void>;
 }
 
 export type TestRunOptions = {
diff --git a/src/client/testing/testController/controller.ts b/src/client/testing/testController/controller.ts
@@ -23,7 +23,7 @@ import { DelayedTrigger, IDelayedTrigger } from '../../common/utils/delayTrigger
 import { sendTelemetryEvent } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
 import { PYTEST_PROVIDER, UNITTEST_PROVIDER } from '../common/constants';
-import { getNodeByUri } from './common/testItemUtilities';
+import { DebugTestTag, getNodeByUri, RunTestTag } from './common/testItemUtilities';
 import { ITestController, ITestFrameworkController, TestRefreshOptions } from './common/types';
 
 @injectable()
@@ -74,12 +74,19 @@ export class PythonTestController implements ITestController {
         this.refreshData = delayTrigger;
 
         this.disposables.push(
-            this.testController.createRunProfile('Run Tests', TestRunProfileKind.Run, this.runTests.bind(this), true),
+            this.testController.createRunProfile(
+                'Run Tests',
+                TestRunProfileKind.Run,
+                this.runTests.bind(this),
+                true,
+                RunTestTag,
+            ),
             this.testController.createRunProfile(
                 'Debug Tests',
                 TestRunProfileKind.Debug,
                 this.runTests.bind(this),
                 true,
+                DebugTestTag,
             ),
         );
         this.testController.resolveHandler = this.resolveChildren.bind(this);
@@ -244,6 +251,7 @@ export class PythonTestController implements ITestController {
                                 },
                                 workspace,
                                 token,
+                                this.testController,
                             );
                         }
                     }
diff --git a/src/client/testing/testController/unittest/runner.ts b/src/client/testing/testController/unittest/runner.ts
@@ -2,15 +2,15 @@
 // Licensed under the MIT License.
 
 import { injectable, inject, named } from 'inversify';
-import { Location, TestItem, TestMessage, TestRun, TestRunProfileKind } from 'vscode';
+import { Location, TestController, TestItem, TestMessage, TestRun, TestRunProfileKind } from 'vscode';
 import { traceError, traceInfo } from '../../../common/logger';
 import * as internalScripts from '../../../common/process/internal/scripts';
 import { IOutputChannel } from '../../../common/types';
 import { noop } from '../../../common/utils/misc';
 import { UNITTEST_PROVIDER } from '../../common/constants';
 import { ITestRunner, ITestDebugLauncher, IUnitTestSocketServer, LaunchOptions, Options } from '../../common/types';
 import { TEST_OUTPUT_CHANNEL } from '../../constants';
-import { getTestCaseNodes } from '../common/testItemUtilities';
+import { clearAllChildren, getTestCaseNodes } from '../common/testItemUtilities';
 import { ITestRun, ITestsRunner, TestData, TestRunInstanceOptions, TestRunOptions } from '../common/types';
 import { fixLogLines } from '../common/utils';
 import { getTestRunArgs } from './arguments';
@@ -20,6 +20,7 @@ interface ITestData {
     message: string;
     outcome: string;
     traceback: string;
+    subtest?: string;
 }
 
 @injectable()
@@ -35,6 +36,7 @@ export class UnittestRunner implements ITestsRunner {
         testRun: ITestRun,
         options: TestRunOptions,
         idToRawData: Map<string, TestData>,
+        testController?: TestController,
     ): Promise<void> {
         const runOptions: TestRunInstanceOptions = {
             ...options,
@@ -43,7 +45,7 @@ export class UnittestRunner implements ITestsRunner {
         };
 
         try {
-            await this.runTest(testRun.includes, testRun.runInstance, runOptions, idToRawData);
+            await this.runTest(testRun.includes, testRun.runInstance, runOptions, idToRawData, testController);
         } catch (ex) {
             testRun.runInstance.appendOutput(`Error while running tests:\r\n${ex}\r\n\r\n`);
         }
@@ -54,6 +56,7 @@ export class UnittestRunner implements ITestsRunner {
         runInstance: TestRun,
         options: TestRunInstanceOptions,
         idToRawData: Map<string, TestData>,
+        testController?: TestController,
     ): Promise<void> {
         runInstance.appendOutput(`Running tests (unittest): ${testNodes.map((t) => t.id).join(' ; ')}\r\n`);
         const testCaseNodes: TestItem[] = [];
@@ -77,12 +80,13 @@ export class UnittestRunner implements ITestsRunner {
         const tested: string[] = [];
 
         const counts = {
-            total: testCaseNodes.length,
+            total: 0,
             passed: 0,
             skipped: 0,
             errored: 0,
             failed: 0,
         };
+        const subTestStats: Map<string, { passed: number; failed: number }> = new Map();
 
         let failFast = false;
         let stopTesting = false;
@@ -98,6 +102,7 @@ export class UnittestRunner implements ITestsRunner {
             const testCase = testCaseNodes.find((node) => idToRawData.get(node.id)?.runId === data.test);
             const rawTestCase = idToRawData.get(testCase?.id ?? '');
             if (testCase && rawTestCase) {
+                counts.total += 1;
                 tested.push(rawTestCase.runId);
 
                 if (data.outcome === 'passed' || data.outcome === 'failed-expected') {
@@ -147,6 +152,70 @@ export class UnittestRunner implements ITestsRunner {
                     runInstance.skipped(testCase);
                     runInstance.appendOutput(fixLogLines(text));
                     counts.skipped += 1;
+                } else if (data.outcome === 'subtest-passed') {
+                    const sub = subTestStats.get(data.test);
+                    if (sub) {
+                        sub.passed += 1;
+                    } else {
+                        counts.passed += 1;
+                        subTestStats.set(data.test, { passed: 1, failed: 0 });
+                        runInstance.appendOutput(fixLogLines(`${rawTestCase.rawId} [subtests]:\r\n`));
+
+                        // We are seeing the first subtest for this node. Clear all other nodes under it
+                        // because we have no way to detect these at discovery, they can always be different
+                        // for each run.
+                        clearAllChildren(testCase);
+                    }
+                    if (data.subtest) {
+                        runInstance.appendOutput(fixLogLines(`${data.subtest} Passed\r\n`));
+
+                        // This is a runtime only node for unittest subtest, since they can only be detected
+                        // at runtime. So, create a fresh one for each result.
+                        const subtest = testController?.createTestItem(data.subtest, data.subtest);
+                        if (subtest) {
+                            testCase.children.add(subtest);
+                            runInstance.started(subtest);
+                            runInstance.passed(subtest);
+                        }
+                    }
+                } else if (data.outcome === 'subtest-failed') {
+                    const sub = subTestStats.get(data.test);
+                    if (sub) {
+                        sub.failed += 1;
+                    } else {
+                        counts.failed += 1;
+                        subTestStats.set(data.test, { passed: 0, failed: 1 });
+
+                        runInstance.appendOutput(fixLogLines(`${rawTestCase.rawId} [subtests]:\r\n`));
+
+                        // We are seeing the first subtest for this node. Clear all other nodes under it
+                        // because we have no way to detect these at discovery, they can always be different
+                        // for each run.
+                        clearAllChildren(testCase);
+                    }
+
+                    if (data.subtest) {
+                        runInstance.appendOutput(fixLogLines(`${data.subtest} Failed\r\n`));
+                        const traceback = data.traceback
+                            ? data.traceback.splitLines({ trim: false, removeEmptyEntries: true }).join('\r\n')
+                            : '';
+                        const text = `${data.subtest} Failed: ${data.message ?? data.outcome}\r\n${traceback}\r\n`;
+                        runInstance.appendOutput(fixLogLines(text));
+
+                        // This is a runtime only node for unittest subtest, since they can only be detected
+                        // at runtime. So, create a fresh one for each result.
+                        const subtest = testController?.createTestItem(data.subtest, data.subtest);
+                        if (subtest) {
+                            testCase.children.add(subtest);
+                            runInstance.started(subtest);
+                            const message = new TestMessage(text);
+                            if (testCase.uri && testCase.range) {
+                                message.location = new Location(testCase.uri, testCase.range);
+                            }
+
+                            runInstance.failed(subtest, message);
+                        }
+                    }
                 } else {
                     const text = `Unknown outcome type for test ${rawTestCase.rawId}: ${data.outcome}`;
                     runInstance.appendOutput(fixLogLines(text));
@@ -233,7 +302,17 @@ export class UnittestRunner implements ITestsRunner {
         runInstance.appendOutput(`Total number of tests passed: ${counts.passed}\r\n`);
         runInstance.appendOutput(`Total number of tests failed: ${counts.failed}\r\n`);
         runInstance.appendOutput(`Total number of tests failed with errors: ${counts.errored}\r\n`);
-        runInstance.appendOutput(`Total number of tests skipped: ${counts.skipped}\r\n`);
+        runInstance.appendOutput(`Total number of tests skipped: ${counts.skipped}\r\n\r\n`);
+
+        if (subTestStats.size > 0) {
+            runInstance.appendOutput('Sub-test stats: \r\n');
+        }
+
+        subTestStats.forEach((v, k) => {
+            runInstance.appendOutput(
+                `Sub-tests for [${k}]: Total=${v.passed + v.failed} Passed=${v.passed} Failed=${v.failed}\r\n\r\n`,
+            );
+        });
 
         if (failFast) {
             runInstance.appendOutput(
diff --git a/src/client/testing/testController/unittest/unittestController.ts b/src/client/testing/testController/unittest/unittestController.ts
@@ -240,7 +240,12 @@ export class UnittestController implements ITestFrameworkController {
         return Promise.resolve();
     }
 
-    public runTests(testRun: ITestRun, workspace: WorkspaceFolder, token: CancellationToken): Promise<void> {
+    public runTests(
+        testRun: ITestRun,
+        workspace: WorkspaceFolder,
+        token: CancellationToken,
+        testController?: TestController,
+    ): Promise<void> {
         const settings = this.configService.getSettings(workspace.uri);
         return this.runner.runTests(
             testRun,
@@ -254,6 +259,7 @@ export class UnittestController implements ITestFrameworkController {
                 args: settings.testing.unittestArgs,
             },
             this.idToRawData,
+            testController,
         );
     }
 }
diff --git a/types/vscode.proposed.d.ts b/types/vscode.proposed.d.ts

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Ensures test node is updated when `unittest` sub-tests are used.