Refactor Python Unit Test functionality to improve unit testing (#1713)

Fixes #1068
DonJayamanne · May 22, 2018 · cfa6f21 · cfa6f21
1 parent 16d990f
commit cfa6f21
Show file tree

Hide file tree

Showing 26 changed files with 1,697 additions and 591 deletions.
diff --git a/news/3 Code Health/1068.md b/news/3 Code Health/1068.md
@@ -0,0 +1 @@
+Refactor unit testing functionality to improve testability of individual components.
diff --git a/src/client/activation/classic.ts b/src/client/activation/classic.ts
@@ -3,7 +3,7 @@
 
 import { DocumentFilter, ExtensionContext, languages, OutputChannel } from 'vscode';
 import { STANDARD_OUTPUT_CHANNEL } from '../common/constants';
-import { IOutputChannel, IPythonSettings } from '../common/types';
+import { ILogger, IOutputChannel, IPythonSettings } from '../common/types';
 import { IShebangCodeLensProvider } from '../interpreter/contracts';
 import { IServiceManager } from '../ioc/types';
 import { JediFactory } from '../languageServices/jediProxyFactory';
@@ -16,8 +16,7 @@ import { PythonRenameProvider } from '../providers/renameProvider';
 import { PythonSignatureProvider } from '../providers/signatureProvider';
 import { activateSimplePythonRefactorProvider } from '../providers/simpleRefactorProvider';
 import { PythonSymbolProvider } from '../providers/symbolProvider';
-import { TEST_OUTPUT_CHANNEL } from '../unittests/common/constants';
-import * as tests from '../unittests/main';
+import { IUnitTestManagementService } from '../unittests/types';
 import { IExtensionActivator } from './types';
 
 export class ClassicExtensionActivator implements IExtensionActivator {
@@ -49,8 +48,10 @@ export class ClassicExtensionActivator implements IExtensionActivator {
  context.subscriptions.push(languages.registerSignatureHelpProvider(this.documentSelector, new PythonSignatureProvider(jediFactory), '(', ','));
  }
 
- const unitTestOutChannel = this.serviceManager.get<OutputChannel>(IOutputChannel, TEST_OUTPUT_CHANNEL);
- tests.activate(context, unitTestOutChannel, symbolProvider, this.serviceManager);
+ const testManagementService = this.serviceManager.get<IUnitTestManagementService>(IUnitTestManagementService);
+ testManagementService.activate()
+ .then(() => testManagementService.activateCodeLenses(symbolProvider))
+ .catch(ex => this.serviceManager.get<ILogger>(ILogger).logError('Failed to activate Unit Tests', ex));
 
  return true;
  }

diff --git a/src/client/unittests/common/managers/baseTestManager.ts b/src/client/unittests/common/managers/baseTestManager.ts
@@ -1,17 +1,13 @@
-import * as vscode from 'vscode';
-import { Disposable, OutputChannel, Uri, workspace } from 'vscode';
+import { CancellationToken, CancellationTokenSource, Disposable, OutputChannel, Uri, workspace } from 'vscode';
 import { PythonSettings } from '../../../common/configSettings';
 import { isNotInstalledError } from '../../../common/helpers';
-import { IPythonSettings } from '../../../common/types';
-import { IDisposableRegistry, IInstaller, IOutputChannel, Product } from '../../../common/types';
+import { IDisposableRegistry, IInstaller, IOutputChannel, IPythonSettings, Product } from '../../../common/types';
 import { IServiceContainer } from '../../../ioc/types';
 import { UNITTEST_DISCOVER, UNITTEST_RUN } from '../../../telemetry/constants';
 import { sendTelemetryEvent } from '../../../telemetry/index';
 import { TestDiscoverytTelemetry, TestRunTelemetry } from '../../../telemetry/types';
 import { CANCELLATION_REASON, CommandSource, TEST_OUTPUT_CHANNEL } from './../constants';
-import { displayTestErrorMessage } from './../testUtils';
-import { ITestCollectionStorageService, ITestDiscoveryService, ITestManager, ITestResultsService } from './../types';
-import { TestDiscoveryOptions, TestProvider, Tests, TestStatus, TestsToRun } from './../types';
+import { ITestCollectionStorageService, ITestDiscoveryService, ITestManager, ITestResultsService, ITestsHelper, TestDiscoveryOptions, TestProvider, Tests, TestStatus, TestsToRun } from './../types';
 
 enum CancellationTokenType {
  testDiscovery,
@@ -33,9 +29,9 @@ export abstract class BaseTestManager implements ITestManager {
  private tests?: Tests;
  // tslint:disable-next-line:variable-name
  private _status: TestStatus = TestStatus.Unknown;
- private testDiscoveryCancellationTokenSource?: vscode.CancellationTokenSource;
- private testRunnerCancellationTokenSource?: vscode.CancellationTokenSource;
- private _installer: IInstaller;
+ private testDiscoveryCancellationTokenSource?: CancellationTokenSource;
+ private testRunnerCancellationTokenSource?: CancellationTokenSource;
+ private _installer!: IInstaller;
  private discoverTestsPromise?: Promise<Tests>;
  private get installer(): IInstaller {
  if (!this._installer) {
@@ -53,10 +49,10 @@ export abstract class BaseTestManager implements ITestManager {
  this.testCollectionStorage = this.serviceContainer.get<ITestCollectionStorageService>(ITestCollectionStorageService);
  this._testResultsService = this.serviceContainer.get<ITestResultsService>(ITestResultsService);
  }
- protected get testDiscoveryCancellationToken(): vscode.CancellationToken | undefined {
+ protected get testDiscoveryCancellationToken(): CancellationToken | undefined {
  return this.testDiscoveryCancellationTokenSource ? this.testDiscoveryCancellationTokenSource.token : undefined;
  }
- protected get testRunnerCancellationToken(): vscode.CancellationToken | undefined {
+ protected get testRunnerCancellationToken(): CancellationToken | undefined {
  return this.testRunnerCancellationTokenSource ? this.testRunnerCancellationTokenSource.token : undefined;
  }
  public dispose() {
@@ -66,7 +62,7 @@ export abstract class BaseTestManager implements ITestManager {
  return this._status;
  }
  public get workingDirectory(): string {
- const settings = PythonSettings.getInstance(vscode.Uri.file(this.rootDirectory));
+ const settings = PythonSettings.getInstance(Uri.file(this.rootDirectory));
  return settings.unitTest.cwd && settings.unitTest.cwd.length > 0 ? settings.unitTest.cwd : this.rootDirectory;
  }
  public stop() {
@@ -133,9 +129,10 @@ export abstract class BaseTestManager implements ITestManager {
  }
  });
  if (haveErrorsInDiscovering && !quietMode) {
- displayTestErrorMessage('There were some errors in discovering unit tests');
+ const testsHelper = this.serviceContainer.get<ITestsHelper>(ITestsHelper);
+ testsHelper.displayTestErrorMessage('There were some errors in discovering unit tests');
  }
- const wkspace = workspace.getWorkspaceFolder(vscode.Uri.file(this.rootDirectory))!.uri;
+ const wkspace = workspace.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
  this.testCollectionStorage.storeTests(wkspace, tests);
  this.disposeCancellationToken(CancellationTokenType.testDiscovery);
  sendTelemetryEvent(UNITTEST_DISCOVER, undefined, telementryProperties);
@@ -159,7 +156,7 @@ export abstract class BaseTestManager implements ITestManager {
  // tslint:disable-next-line:prefer-template
  this.outputChannel.appendLine(reason.toString());
  }
- const wkspace = workspace.getWorkspaceFolder(vscode.Uri.file(this.rootDirectory))!.uri;
+ const wkspace = workspace.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
  this.testCollectionStorage.storeTests(wkspace, null);
  this.disposeCancellationToken(CancellationTokenType.testDiscovery);
  return Promise.reject(reason);
@@ -217,8 +214,9 @@ export abstract class BaseTestManager implements ITestManager {
  if (this.testDiscoveryCancellationToken && this.testDiscoveryCancellationToken.isCancellationRequested) {
  return Promise.reject<Tests>(reason);
  }
- displayTestErrorMessage('Errors in discovering tests, continuing with tests');
- return <Tests>{
+ const testsHelper = this.serviceContainer.get<ITestsHelper>(ITestsHelper);
+ testsHelper.displayTestErrorMessage('Errors in discovering tests, continuing with tests');
+ return {
  rootTestFolders: [], testFiles: [], testFolders: [], testFunctions: [], testSuites: [],
  summary: { errors: 0, failures: 0, passed: 0, skipped: 0 }
  };
@@ -250,9 +248,9 @@ export abstract class BaseTestManager implements ITestManager {
  private createCancellationToken(tokenType: CancellationTokenType) {
  this.disposeCancellationToken(tokenType);
  if (tokenType === CancellationTokenType.testDiscovery) {
- this.testDiscoveryCancellationTokenSource = new vscode.CancellationTokenSource();
+ this.testDiscoveryCancellationTokenSource = new CancellationTokenSource();
  } else {
- this.testRunnerCancellationTokenSource = new vscode.CancellationTokenSource();
+ this.testRunnerCancellationTokenSource = new CancellationTokenSource();
  }
  }
  private disposeCancellationToken(tokenType: CancellationTokenType) {

diff --git a/src/client/unittests/common/managers/testConfigurationManager.ts b/src/client/unittests/common/managers/testConfigurationManager.ts
@@ -1,18 +1,26 @@
 import * as path from 'path';
 import { OutputChannel, QuickPickItem, Uri, window } from 'vscode';
 import { createDeferred } from '../../../common/helpers';
-import { IInstaller, Product } from '../../../common/types';
+import { IInstaller, IOutputChannel, Product } from '../../../common/types';
 import { getSubDirectories } from '../../../common/utils';
+import { IServiceContainer } from '../../../ioc/types';
+import { ITestConfigurationManager } from '../../types';
+import { TEST_OUTPUT_CHANNEL } from '../constants';
 import { ITestConfigSettingsService, UnitTestProduct } from './../types';
 
-export abstract class TestConfigurationManager {
+export abstract class TestConfigurationManager implements ITestConfigurationManager {
+ protected readonly outputChannel: OutputChannel;
+ protected readonly installer: IInstaller;
+ protected readonly testConfigSettingsService: ITestConfigSettingsService;
  constructor(protected workspace: Uri,
  protected product: UnitTestProduct,
- protected readonly outputChannel: OutputChannel,
- protected installer: IInstaller,
- protected testConfigSettingsService: ITestConfigSettingsService) { }
- // tslint:disable-next-line:no-any
- public abstract configure(wkspace: Uri): Promise<any>;
+ protected readonly serviceContainer: IServiceContainer) {
+ this.outputChannel = serviceContainer.get<OutputChannel>(IOutputChannel, TEST_OUTPUT_CHANNEL);
+ this.installer = serviceContainer.get<IInstaller>(IInstaller);
+ this.testConfigSettingsService = serviceContainer.get<ITestConfigSettingsService>(ITestConfigSettingsService);
+ }
+ public abstract configure(wkspace: Uri): Promise<void>;
+ public abstract requiresUserToConfigure(wkspace: Uri): Promise<boolean>;
  public async enable() {
  // Disable other test frameworks.
  const testProducsToDisable = [Product.pytest, Product.unittest, Product.nosetest]

diff --git a/src/client/unittests/common/services/configSettingService.ts b/src/client/unittests/common/services/configSettingService.ts
@@ -1,9 +1,31 @@
-import { Uri, workspace, WorkspaceConfiguration } from 'vscode';
+import { inject, injectable } from 'inversify';
+import { Uri, WorkspaceConfiguration } from 'vscode';
+import { IWorkspaceService } from '../../../common/application/types';
 import { Product } from '../../../common/types';
+import { IServiceContainer } from '../../../ioc/types';
 import { ITestConfigSettingsService, UnitTestProduct } from './../types';
 
+@injectable()
 export class TestConfigSettingsService implements ITestConfigSettingsService {
- private static getTestArgSetting(product: UnitTestProduct) {
+ private readonly workspaceService: IWorkspaceService;
+ constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
+ this.workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
+ }
+ public async updateTestArgs(testDirectory: string | Uri, product: UnitTestProduct, args: string[]) {
+ const setting = this.getTestArgSetting(product);
+ return this.updateSetting(testDirectory, setting, args);
+ }
+
+ public async enable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
+ const setting = this.getTestEnablingSetting(product);
+ return this.updateSetting(testDirectory, setting, true);
+ }
+
+ public async disable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
+ const setting = this.getTestEnablingSetting(product);
+ return this.updateSetting(testDirectory, setting, false);
+ }
+ private getTestArgSetting(product: UnitTestProduct) {
  switch (product) {
  case Product.unittest:
  return 'unitTest.unittestArgs';
@@ -15,7 +37,7 @@ export class TestConfigSettingsService implements ITestConfigSettingsService {
  throw new Error('Invalid Test Product');
  }
  }
- private static getTestEnablingSetting(product: UnitTestProduct) {
+ private getTestEnablingSetting(product: UnitTestProduct) {
  switch (product) {
  case Product.unittest:
  return 'unitTest.unittestEnabled';
@@ -28,36 +50,22 @@ export class TestConfigSettingsService implements ITestConfigSettingsService {
  }
  }
  // tslint:disable-next-line:no-any
- private static async updateSetting(testDirectory: string | Uri, setting: string, value: any) {
+ private async updateSetting(testDirectory: string | Uri, setting: string, value: any) {
  let pythonConfig: WorkspaceConfiguration;
  const resource = typeof testDirectory === 'string' ? Uri.file(testDirectory) : testDirectory;
- if (!Array.isArray(workspace.workspaceFolders) || workspace.workspaceFolders.length === 0) {
- pythonConfig = workspace.getConfiguration('python');
- } else if (workspace.workspaceFolders.length === 1) {
- pythonConfig = workspace.getConfiguration('python', workspace.workspaceFolders[0].uri);
+ if (!this.workspaceService.hasWorkspaceFolders) {
+ pythonConfig = this.workspaceService.getConfiguration('python');
+ } else if (this.workspaceService.workspaceFolders!.length === 1) {
+ pythonConfig = this.workspaceService.getConfiguration('python', this.workspaceService.workspaceFolders![0].uri);
  } else {
- const workspaceFolder = workspace.getWorkspaceFolder(resource);
+ const workspaceFolder = this.workspaceService.getWorkspaceFolder(resource);
  if (!workspaceFolder) {
  throw new Error(`Test directory does not belong to any workspace (${testDirectory})`);
  }
  // tslint:disable-next-line:no-non-null-assertion
- pythonConfig = workspace.getConfiguration('python', workspaceFolder!.uri);
+ pythonConfig = this.workspaceService.getConfiguration('python', workspaceFolder!.uri);
  }
 
  return pythonConfig.update(setting, value);
  }
- public async updateTestArgs(testDirectory: string | Uri, product: UnitTestProduct, args: string[]) {
- const setting = TestConfigSettingsService.getTestArgSetting(product);
- return TestConfigSettingsService.updateSetting(testDirectory, setting, args);
- }
-
- public async enable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
- const setting = TestConfigSettingsService.getTestEnablingSetting(product);
- return TestConfigSettingsService.updateSetting(testDirectory, setting, true);
- }
-
- public async disable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
- const setting = TestConfigSettingsService.getTestEnablingSetting(product);
- return TestConfigSettingsService.updateSetting(testDirectory, setting, false);
- }
 }
diff --git a/src/client/unittests/common/testUtils.ts b/src/client/unittests/common/testUtils.ts
@@ -1,8 +1,10 @@
 import { inject, injectable, named } from 'inversify';
 import * as path from 'path';
-import { commands, Uri, window, workspace } from 'vscode';
+import { Uri, window, workspace } from 'vscode';
+import { IApplicationShell, ICommandManager } from '../../common/application/types';
 import * as constants from '../../common/constants';
 import { IUnitTestSettings, Product } from '../../common/types';
+import { IServiceContainer } from '../../ioc/types';
 import { CommandSource } from './constants';
 import { TestFlatteningVisitor } from './testVisitors/flatteningVisitor';
 import { ITestsHelper, ITestVisitor, TestFile, TestFolder, TestProvider, Tests, TestSettingsPropertyNames, TestsToRun, UnitTestProduct } from './types';
@@ -19,15 +21,6 @@ export async function selectTestWorkspace(): Promise<Uri | undefined> {
  }
 }
 
-export function displayTestErrorMessage(message: string) {
- window.showErrorMessage(message, constants.Button_Text_Tests_View_Output).then(action => {
- if (action === constants.Button_Text_Tests_View_Output) {
- commands.executeCommand(constants.Commands.Tests_ViewOutput, undefined, CommandSource.ui);
- }
- });
-
-}
-
 export function extractBetweenDelimiters(content: string, startDelimiter: string, endDelimiter: string): string {
  content = content.substring(content.indexOf(startDelimiter) + startDelimiter.length);
  return content.substring(0, content.lastIndexOf(endDelimiter));
@@ -40,7 +33,13 @@ export function convertFileToPackage(filePath: string): string {
 
 @injectable()
 export class TestsHelper implements ITestsHelper {
- constructor(@inject(ITestVisitor) @named('TestFlatteningVisitor') private flatteningVisitor: TestFlatteningVisitor) { }
+ private readonly appShell: IApplicationShell;
+ private readonly commandManager: ICommandManager;
+ constructor(@inject(ITestVisitor) @named('TestFlatteningVisitor') private flatteningVisitor: TestFlatteningVisitor,
+ @inject(IServiceContainer) serviceContainer: IServiceContainer) {
+ this.appShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
+ this.commandManager = serviceContainer.get<ICommandManager>(ICommandManager);
+ }
  public parseProviderName(product: UnitTestProduct): TestProvider {
  switch (product) {
  case Product.nosetest: return 'nosetest';
@@ -165,4 +164,11 @@ export class TestsHelper implements ITestsHelper {
  // tslint:disable-next-line:no-object-literal-type-assertion
  return <TestsToRun>{ testFile: [{ name: name, nameToRun: name, functions: [], suites: [], xmlName: name, fullPath: '', time: 0 }] };
  }
+ public displayTestErrorMessage(message: string) {
+ this.appShell.showErrorMessage(message, constants.Button_Text_Tests_View_Output).then(action => {
+ if (action === constants.Button_Text_Tests_View_Output) {
+ this.commandManager.executeCommand(constants.Commands.Tests_ViewOutput, undefined, CommandSource.ui);
+ }
+ });
+ }
 }