Preserve status of tests when re-discovering automatically (#4638)

DonJayamanne · web-flow · commit d4ea4b61361d · 2019-03-07T10:55:28.000-08:00
* Display test explorer when discovery has been run
* Preserve state when adding removing tests
* Retain state of tests when auto discovering tests
* Delete unwanted test
* Fix test
* Address code review comments
* Remove unnecessary file
* Fixed broken tests
diff --git a/news/1 Enhancements/4576.md b/news/1 Enhancements/4576.md
@@ -0,0 +1 @@
+Retain state of tests when auto discovering tests.
diff --git a/src/client/unittests/common/managers/baseTestManager.ts b/src/client/unittests/common/managers/baseTestManager.ts
@@ -14,6 +14,7 @@ import {
 import { ICommandManager, IWorkspaceService } from '../../../common/application/types';
 import '../../../common/extensions';
 import { isNotInstalledError } from '../../../common/helpers';
+import { traceError } from '../../../common/logger';
 import { IFileSystem } from '../../../common/platform/types';
 import { IConfigurationService, IDisposableRegistry, IInstaller, IOutputChannel, IPythonSettings, Product } from '../../../common/types';
 import { getNamesAndValues } from '../../../common/utils/enum';
@@ -23,14 +24,15 @@ import { EventName } from '../../../telemetry/constants';
 import { sendTelemetryEvent } from '../../../telemetry/index';
 import { TestDiscoverytTelemetry, TestRunTelemetry } from '../../../telemetry/types';
 import { IPythonUnitTestMessage, IUnitTestDiagnosticService, WorkspaceTestStatus } from '../../types';
-import { ITestsStatusUpdaterService } from '../types';
+import { copyTestResults } from '../testUtils';
 import { CANCELLATION_REASON, CommandSource, TEST_OUTPUT_CHANNEL } from './../constants';
 import {
     ITestCollectionStorageService,
     ITestDiscoveryService,
     ITestManager,
     ITestResultsService,
     ITestsHelper,
+    ITestsStatusUpdaterService,
     TestDiscoveryOptions,
     TestProvider,
     Tests,
@@ -56,19 +58,19 @@ export abstract class BaseTestManager implements ITestManager {
     protected get testResultsService() {
         return this._testResultsService;
     }
-    private testCollectionStorage: ITestCollectionStorageService;
-    private _testResultsService: ITestResultsService;
-    private commandManager: ICommandManager;
-    private workspaceService: IWorkspaceService;
-    private _outputChannel: OutputChannel;
+    private readonly testCollectionStorage: ITestCollectionStorageService;
+    private readonly _testResultsService: ITestResultsService;
+    private readonly commandManager: ICommandManager;
+    private readonly workspaceService: IWorkspaceService;
+    private readonly _outputChannel: OutputChannel;
     protected tests?: Tests;
     private _status: TestStatus = TestStatus.Unknown;
     private testDiscoveryCancellationTokenSource?: CancellationTokenSource;
     private testRunnerCancellationTokenSource?: CancellationTokenSource;
     private _installer!: IInstaller;
-    private testsStatusUpdaterService: ITestsStatusUpdaterService;
+    private readonly testsStatusUpdaterService: ITestsStatusUpdaterService;
     private discoverTestsPromise?: Promise<Tests>;
-    private _onDidStatusChange = new EventEmitter<WorkspaceTestStatus>();
+    private readonly _onDidStatusChange = new EventEmitter<WorkspaceTestStatus>();
     private get installer(): IInstaller {
         if (!this._installer) {
             this._installer = this.serviceContainer.get<IInstaller>(IInstaller);
@@ -77,7 +79,7 @@ export abstract class BaseTestManager implements ITestManager {
     }
     constructor(
         public readonly testProvider: TestProvider,
-        private product: Product,
+        private readonly product: Product,
         public readonly workspaceFolder: Uri,
         protected rootDirectory: string,
         protected serviceContainer: IServiceContainer
@@ -133,15 +135,15 @@ export abstract class BaseTestManager implements ITestManager {
 
         this.testResultsService.resetResults(this.tests!);
     }
-    public async discoverTests(cmdSource: CommandSource, ignoreCache: boolean = false, quietMode: boolean = false, userInitiated: boolean = false): Promise<Tests> {
+    public async discoverTests(cmdSource: CommandSource, ignoreCache: boolean = false, quietMode: boolean = false, userInitiated: boolean = false, clearTestStatus: boolean = false): Promise<Tests> {
         if (this.discoverTestsPromise) {
             return this.discoverTestsPromise;
         }
-        this.discoverTestsPromise = this._discoverTests(cmdSource, ignoreCache, quietMode, userInitiated);
+        this.discoverTestsPromise = this._discoverTests(cmdSource, ignoreCache, quietMode, userInitiated, clearTestStatus);
         this.discoverTestsPromise.catch(noop).then(() => this.discoverTestsPromise = undefined).ignoreErrors();
         return this.discoverTestsPromise;
     }
-    private async _discoverTests(cmdSource: CommandSource, ignoreCache: boolean = false, quietMode: boolean = false, userInitiated: boolean = false): Promise<Tests> {
+    private async _discoverTests(cmdSource: CommandSource, ignoreCache: boolean = false, quietMode: boolean = false, userInitiated: boolean = false, clearTestStatus: boolean = false): Promise<Tests> {
         if (!ignoreCache && this.tests! && this.tests!.testFunctions.length > 0) {
             this.updateStatus(TestStatus.Idle);
             return Promise.resolve(this.tests!);
@@ -168,8 +170,16 @@ export abstract class BaseTestManager implements ITestManager {
         return discoveryService
             .discoverTests(discoveryOptions)
             .then(tests => {
+                const wkspace = this.workspaceService.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
+                const existingTests = this.testCollectionStorage.getTests(wkspace)!;
+                if (clearTestStatus) {
+                    this.resetTestResults();
+                } else if (existingTests) {
+                    copyTestResults(existingTests, tests);
+                    this._testResultsService.updateResults(tests);
+                }
+                this.testCollectionStorage.storeTests(wkspace, tests);
                 this.tests = tests;
-                this.resetTestResults();
                 this.updateStatus(TestStatus.Idle);
                 this.discoverTestsPromise = undefined;
 
@@ -188,8 +198,6 @@ export abstract class BaseTestManager implements ITestManager {
                     const testsHelper = this.serviceContainer.get<ITestsHelper>(ITestsHelper);
                     testsHelper.displayTestErrorMessage('There were some errors in discovering unit tests');
                 }
-                const wkspace = this.workspaceService.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
-                this.testCollectionStorage.storeTests(wkspace, tests);
                 this.disposeCancellationToken(CancellationTokenType.testDiscovery);
                 sendTelemetryEvent(EventName.UNITTEST_DISCOVER, undefined, telementryProperties);
                 return tests;
@@ -200,7 +208,7 @@ export abstract class BaseTestManager implements ITestManager {
                 }
                 if (isNotInstalledError(reason as Error) && !quietMode) {
                     this.installer.promptToInstall(this.product, this.workspaceFolder)
-                        .catch(ex => console.error('Python Extension: isNotInstalledError', ex));
+                        .catch(ex => traceError('isNotInstalledError', ex));
                 }
 
                 this.tests = undefined;
@@ -216,7 +224,7 @@ export abstract class BaseTestManager implements ITestManager {
                     this.outputChannel.appendLine(reason.toString());
                 }
                 const wkspace = this.workspaceService.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
-                this.testCollectionStorage.storeTests(wkspace, null);
+                this.testCollectionStorage.storeTests(wkspace, undefined);
                 this.disposeCancellationToken(CancellationTokenType.testDiscovery);
                 return Promise.reject(reason);
             });
@@ -240,7 +248,6 @@ export abstract class BaseTestManager implements ITestManager {
         };
 
         if (!runFailedTests && !testsToRun) {
-            this.resetTestResults();
             this.testsStatusUpdaterService.updateStatusAsRunning(this.workspaceFolder, this.tests);
         }
 
@@ -419,7 +426,7 @@ export abstract class BaseTestManager implements ITestManager {
         const stackStart = message.locationStack![0];
         const diagPrefix = this.unitTestDiagnosticService.getMessagePrefix(message.status!);
         const severity = this.unitTestDiagnosticService.getSeverity(message.severity)!;
-        const diagMsg = message.message!.split('\n')[0];
+        const diagMsg = message.message ? message.message.split('\n')[0] : '';
         const diagnostic = new Diagnostic(stackStart.location.range, `${diagPrefix ? `${diagPrefix}: ` : ''}${diagMsg}`, severity);
         diagnostic.code = message.code;
         diagnostic.source = message.provider;
diff --git a/src/client/unittests/common/services/storageService.ts b/src/client/unittests/common/services/storageService.ts
@@ -1,28 +1,30 @@
 import { inject, injectable } from 'inversify';
-import { Disposable, Event, EventEmitter, Uri, workspace } from 'vscode';
+import { Disposable, Event, EventEmitter, Uri } from 'vscode';
+import { IWorkspaceService } from '../../../common/application/types';
 import { IDisposableRegistry } from '../../../common/types';
 import { TestDataItem } from '../../types';
 import { FlattenedTestFunction, FlattenedTestSuite, ITestCollectionStorageService, TestFunction, Tests, TestSuite } from './../types';
 
 @injectable()
 export class TestCollectionStorageService implements ITestCollectionStorageService {
-    private _onDidChange = new EventEmitter<{ uri: Uri; data?: TestDataItem }>();
-    private testsIndexedByWorkspaceUri = new Map<string, Tests | undefined>();
+    private readonly _onDidChange = new EventEmitter<{ uri: Uri; data?: TestDataItem }>();
+    private readonly testsIndexedByWorkspaceUri = new Map<string, Tests | undefined>();
 
-    constructor(@inject(IDisposableRegistry) disposables: Disposable[]) {
+    constructor(@inject(IDisposableRegistry) disposables: Disposable[],
+        @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService) {
         disposables.push(this);
     }
     public get onDidChange(): Event<{ uri: Uri; data?: TestDataItem }> {
         return this._onDidChange.event;
     }
-    public getTests(wkspace: Uri): Tests | undefined {
-        const workspaceFolder = this.getWorkspaceFolderPath(wkspace) || '';
+    public getTests(resource: Uri): Tests | undefined {
+        const workspaceFolder = this.workspaceService.getWorkspaceFolderIdentifier(resource);
         return this.testsIndexedByWorkspaceUri.has(workspaceFolder) ? this.testsIndexedByWorkspaceUri.get(workspaceFolder) : undefined;
     }
-    public storeTests(wkspace: Uri, tests: Tests | undefined): void {
-        const workspaceFolder = this.getWorkspaceFolderPath(wkspace) || '';
+    public storeTests(resource: Uri, tests: Tests | undefined): void {
+        const workspaceFolder = this.workspaceService.getWorkspaceFolderIdentifier(resource);
         this.testsIndexedByWorkspaceUri.set(workspaceFolder, tests);
-        this._onDidChange.fire({ uri: wkspace });
+        this._onDidChange.fire({ uri: resource });
     }
     public findFlattendTestFunction(resource: Uri, func: TestFunction): FlattenedTestFunction | undefined {
         const tests = this.getTests(resource);
@@ -41,11 +43,7 @@ export class TestCollectionStorageService implements ITestCollectionStorageServi
     public dispose() {
         this.testsIndexedByWorkspaceUri.clear();
     }
-    public update(resource: Uri, item: TestDataItem): void{
+    public update(resource: Uri, item: TestDataItem): void {
         this._onDidChange.fire({ uri: resource, data: item });
     }
-    private getWorkspaceFolderPath(resource: Uri): string | undefined {
-        const folder = workspace.getWorkspaceFolder(resource);
-        return folder ? folder.uri.path : undefined;
-    }
 }
diff --git a/src/client/unittests/common/testUtils.ts b/src/client/unittests/common/testUtils.ts
@@ -54,7 +54,7 @@ export class TestsHelper implements ITestsHelper {
     private readonly appShell: IApplicationShell;
     private readonly commandManager: ICommandManager;
     constructor(
-        @inject(ITestVisitor) @named('TestFlatteningVisitor') private flatteningVisitor: TestFlatteningVisitor,
+        @inject(ITestVisitor) @named('TestFlatteningVisitor') private readonly flatteningVisitor: TestFlatteningVisitor,
         @inject(IServiceContainer) serviceContainer: IServiceContainer
     ) {
         this.appShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
@@ -451,3 +451,62 @@ export function getChildren(item: TestDataItem): TestDataItem[] {
         }
     }
 }
+
+export function copyTestResults(source: Tests, target: Tests): void {
+    copyResultsForFolders(source.testFolders, target.testFolders);
+}
+
+function copyResultsForFolders(source: TestFolder[], target: TestFolder[]): void {
+    source.forEach(sourceFolder => {
+        const targetFolder = target.find(folder => folder.name === sourceFolder.name && folder.nameToRun === sourceFolder.nameToRun);
+        if (!targetFolder) {
+            return;
+        }
+        copyValueTypes<TestFolder>(sourceFolder, targetFolder);
+        copyResultsForFiles(sourceFolder.testFiles, targetFolder.testFiles);
+    });
+}
+function copyResultsForFiles(source: TestFile[], target: TestFile[]): void {
+    source.forEach(sourceFile => {
+        const targetFile = target.find(file => file.name === sourceFile.name && file.nameToRun === sourceFile.nameToRun);
+        if (!targetFile) {
+            return;
+        }
+        copyValueTypes<TestFile>(sourceFile, targetFile);
+        copyResultsForFunctions(sourceFile.functions, targetFile.functions);
+        copyResultsForSuites(sourceFile.suites, targetFile.suites);
+    });
+}
+
+function copyResultsForFunctions(source: TestFunction[], target: TestFunction[]): void {
+    source.forEach(sourceFn => {
+        const targetFn = target.find(fn => fn.name === sourceFn.name && fn.nameToRun === sourceFn.nameToRun);
+        if (!targetFn) {
+            return;
+        }
+        copyValueTypes<TestFunction>(sourceFn, targetFn);
+    });
+}
+
+function copyResultsForSuites(source: TestSuite[], target: TestSuite[]): void {
+    source.forEach(sourceSuite => {
+        const targetSuite = target.find(suite => suite.name === sourceSuite.name &&
+            suite.nameToRun === sourceSuite.nameToRun &&
+            suite.xmlName === sourceSuite.xmlName);
+        if (!targetSuite) {
+            return;
+        }
+        copyValueTypes<TestSuite>(sourceSuite, targetSuite);
+        copyResultsForFunctions(sourceSuite.functions, targetSuite.functions);
+        copyResultsForSuites(sourceSuite.suites, targetSuite.suites);
+    });
+}
+
+function copyValueTypes<T>(source: T, target: T): void {
+    Object.keys(source).forEach(key => {
+        const value = source[key];
+        if (['boolean', 'number', 'string', 'undefined'].indexOf(typeof value) >= 0) {
+            target[key] = value;
+        }
+    });
+}
diff --git a/src/client/unittests/common/types.ts b/src/client/unittests/common/types.ts
@@ -232,7 +232,7 @@ export interface ITestManager extends Disposable {
     readonly onDidStatusChange: Event<WorkspaceTestStatus>;
     stop(): void;
     resetTestResults(): void;
-    discoverTests(cmdSource: CommandSource, ignoreCache?: boolean, quietMode?: boolean, userInitiated?: boolean): Promise<Tests>;
+    discoverTests(cmdSource: CommandSource, ignoreCache?: boolean, quietMode?: boolean, userInitiated?: boolean, clearTestStatus?: boolean): Promise<Tests>;
     runTest(cmdSource: CommandSource, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise<Tests>;
 }
 
diff --git a/src/client/unittests/main.ts b/src/client/unittests/main.ts
@@ -174,7 +174,7 @@ export class UnitTestManagementService implements IUnitTestManagementService, Di
 
         this.discoverTests(CommandSource.auto, this.workspaceService.workspaceFolders![0].uri, true).ignoreErrors();
     }
-    public async discoverTests(cmdSource: CommandSource, resource?: Uri, ignoreCache?: boolean, userInitiated?: boolean, quietMode?: boolean) {
+    public async discoverTests(cmdSource: CommandSource, resource?: Uri, ignoreCache?: boolean, userInitiated?: boolean, quietMode?: boolean, clearTestStatus?: boolean) {
         const testManager = await this.getTestManager(true, resource);
         if (!testManager) {
             return;
@@ -187,7 +187,7 @@ export class UnitTestManagementService implements IUnitTestManagementService, Di
         if (!this.testResultDisplay) {
             this.testResultDisplay = this.serviceContainer.get<ITestResultDisplay>(ITestResultDisplay);
         }
-        const discoveryPromise = testManager.discoverTests(cmdSource, ignoreCache, quietMode, userInitiated);
+        const discoveryPromise = testManager.discoverTests(cmdSource, ignoreCache, quietMode, userInitiated, clearTestStatus);
         this.testResultDisplay.displayDiscoverStatus(discoveryPromise, quietMode)
             .catch(ex => console.error('Python Extension: displayDiscoverStatus', ex));
         await discoveryPromise;
@@ -354,7 +354,7 @@ export class UnitTestManagementService implements IUnitTestManagementService, Di
             commandManager.registerCommand(constants.Commands.Tests_Discover, (_, cmdSource: CommandSource = CommandSource.commandPalette, resource?: Uri) => {
                 // Ignore the exceptions returned.
                 // This command will be invoked from other places of the extension.
-                this.discoverTests(cmdSource, resource, true, true)
+                this.discoverTests(cmdSource, resource, true, true, false, true)
                     .ignoreErrors();
             }),
             commandManager.registerCommand(constants.Commands.Tests_Configure, (_, cmdSource: CommandSource = CommandSource.commandPalette, resource?: Uri) => {
diff --git a/src/test/unittests/common/services/storageService.unit.test.ts b/src/test/unittests/common/services/storageService.unit.test.ts
diff --git a/src/test/unittests/common/testUtils.unit.test.ts b/src/test/unittests/common/testUtils.unit.test.ts

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Retain state of tests when auto discovering tests.`
Original file line number	Diff line number	Diff line change
`@@ -232,7 +232,7 @@ export interface ITestManager extends Disposable {`
`232`	`232`	`readonly onDidStatusChange: Event<WorkspaceTestStatus>;`
`233`	`233`	`stop(): void;`
`234`	`234`	`resetTestResults(): void;`
`235`		`- discoverTests(cmdSource: CommandSource, ignoreCache?: boolean, quietMode?: boolean, userInitiated?: boolean): Promise<Tests>;`
	`235`	`+ discoverTests(cmdSource: CommandSource, ignoreCache?: boolean, quietMode?: boolean, userInitiated?: boolean, clearTestStatus?: boolean): Promise<Tests>;`
`236`	`236`	`runTest(cmdSource: CommandSource, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise<Tests>;`
`237`	`237`	`}`
`238`	`238`