Skip to content

Commit cfa6f21

Browse files
DonJayamanneDonJayamanne
authored
Refactor Python Unit Test functionality to improve unit testing (DonJayamanne#1713)
Fixes DonJayamanne#1068
1 parent 16d990f commit cfa6f21

26 files changed

Lines changed: 1697 additions & 591 deletions

‎news/3 Code Health/1068.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Refactor unit testing functionality to improve testability of individual components.

‎src/client/activation/classic.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import { DocumentFilter, ExtensionContext, languages, OutputChannel } from 'vscode';
55
import { STANDARD_OUTPUT_CHANNEL } from '../common/constants';
6-
import { IOutputChannel, IPythonSettings } from '../common/types';
6+
import { ILogger, IOutputChannel, IPythonSettings } from '../common/types';
77
import { IShebangCodeLensProvider } from '../interpreter/contracts';
88
import { IServiceManager } from '../ioc/types';
99
import { JediFactory } from '../languageServices/jediProxyFactory';
@@ -16,8 +16,7 @@ import { PythonRenameProvider } from '../providers/renameProvider';
1616
import { PythonSignatureProvider } from '../providers/signatureProvider';
1717
import { activateSimplePythonRefactorProvider } from '../providers/simpleRefactorProvider';
1818
import { PythonSymbolProvider } from '../providers/symbolProvider';
19-
import { TEST_OUTPUT_CHANNEL } from '../unittests/common/constants';
20-
import * as tests from '../unittests/main';
19+
import { IUnitTestManagementService } from '../unittests/types';
2120
import { IExtensionActivator } from './types';
2221

2322
export class ClassicExtensionActivator implements IExtensionActivator {
@@ -49,8 +48,10 @@ export class ClassicExtensionActivator implements IExtensionActivator {
4948
context.subscriptions.push(languages.registerSignatureHelpProvider(this.documentSelector, new PythonSignatureProvider(jediFactory), '(', ','));
5049
}
5150

52-
const unitTestOutChannel = this.serviceManager.get<OutputChannel>(IOutputChannel, TEST_OUTPUT_CHANNEL);
53-
tests.activate(context, unitTestOutChannel, symbolProvider, this.serviceManager);
51+
const testManagementService = this.serviceManager.get<IUnitTestManagementService>(IUnitTestManagementService);
52+
testManagementService.activate()
53+
.then(() => testManagementService.activateCodeLenses(symbolProvider))
54+
.catch(ex => this.serviceManager.get<ILogger>(ILogger).logError('Failed to activate Unit Tests', ex));
5455

5556
return true;
5657
}

‎src/client/unittests/common/managers/baseTestManager.ts‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,13 @@
1-
import * as vscode from 'vscode';
2-
import { Disposable, OutputChannel, Uri, workspace } from 'vscode';
1+
import { CancellationToken, CancellationTokenSource, Disposable, OutputChannel, Uri, workspace } from 'vscode';
32
import { PythonSettings } from '../../../common/configSettings';
43
import { isNotInstalledError } from '../../../common/helpers';
5-
import { IPythonSettings } from '../../../common/types';
6-
import { IDisposableRegistry, IInstaller, IOutputChannel, Product } from '../../../common/types';
4+
import { IDisposableRegistry, IInstaller, IOutputChannel, IPythonSettings, Product } from '../../../common/types';
75
import { IServiceContainer } from '../../../ioc/types';
86
import { UNITTEST_DISCOVER, UNITTEST_RUN } from '../../../telemetry/constants';
97
import { sendTelemetryEvent } from '../../../telemetry/index';
108
import { TestDiscoverytTelemetry, TestRunTelemetry } from '../../../telemetry/types';
119
import { CANCELLATION_REASON, CommandSource, TEST_OUTPUT_CHANNEL } from './../constants';
12-
import { displayTestErrorMessage } from './../testUtils';
13-
import { ITestCollectionStorageService, ITestDiscoveryService, ITestManager, ITestResultsService } from './../types';
14-
import { TestDiscoveryOptions, TestProvider, Tests, TestStatus, TestsToRun } from './../types';
10+
import { ITestCollectionStorageService, ITestDiscoveryService, ITestManager, ITestResultsService, ITestsHelper, TestDiscoveryOptions, TestProvider, Tests, TestStatus, TestsToRun } from './../types';
1511

1612
enum CancellationTokenType {
1713
testDiscovery,
@@ -33,9 +29,9 @@ export abstract class BaseTestManager implements ITestManager {
3329
private tests?: Tests;
3430
// tslint:disable-next-line:variable-name
3531
private _status: TestStatus = TestStatus.Unknown;
36-
private testDiscoveryCancellationTokenSource?: vscode.CancellationTokenSource;
37-
private testRunnerCancellationTokenSource?: vscode.CancellationTokenSource;
38-
private _installer: IInstaller;
32+
private testDiscoveryCancellationTokenSource?: CancellationTokenSource;
33+
private testRunnerCancellationTokenSource?: CancellationTokenSource;
34+
private _installer!: IInstaller;
3935
private discoverTestsPromise?: Promise<Tests>;
4036
private get installer(): IInstaller {
4137
if (!this._installer) {
@@ -53,10 +49,10 @@ export abstract class BaseTestManager implements ITestManager {
5349
this.testCollectionStorage = this.serviceContainer.get<ITestCollectionStorageService>(ITestCollectionStorageService);
5450
this._testResultsService = this.serviceContainer.get<ITestResultsService>(ITestResultsService);
5551
}
56-
protected get testDiscoveryCancellationToken(): vscode.CancellationToken | undefined {
52+
protected get testDiscoveryCancellationToken(): CancellationToken | undefined {
5753
return this.testDiscoveryCancellationTokenSource ? this.testDiscoveryCancellationTokenSource.token : undefined;
5854
}
59-
protected get testRunnerCancellationToken(): vscode.CancellationToken | undefined {
55+
protected get testRunnerCancellationToken(): CancellationToken | undefined {
6056
return this.testRunnerCancellationTokenSource ? this.testRunnerCancellationTokenSource.token : undefined;
6157
}
6258
public dispose() {
@@ -66,7 +62,7 @@ export abstract class BaseTestManager implements ITestManager {
6662
return this._status;
6763
}
6864
public get workingDirectory(): string {
69-
const settings = PythonSettings.getInstance(vscode.Uri.file(this.rootDirectory));
65+
const settings = PythonSettings.getInstance(Uri.file(this.rootDirectory));
7066
return settings.unitTest.cwd && settings.unitTest.cwd.length > 0 ? settings.unitTest.cwd : this.rootDirectory;
7167
}
7268
public stop() {
@@ -133,9 +129,10 @@ export abstract class BaseTestManager implements ITestManager {
133129
}
134130
});
135131
if (haveErrorsInDiscovering && !quietMode) {
136-
displayTestErrorMessage('There were some errors in discovering unit tests');
132+
const testsHelper = this.serviceContainer.get<ITestsHelper>(ITestsHelper);
133+
testsHelper.displayTestErrorMessage('There were some errors in discovering unit tests');
137134
}
138-
const wkspace = workspace.getWorkspaceFolder(vscode.Uri.file(this.rootDirectory))!.uri;
135+
const wkspace = workspace.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
139136
this.testCollectionStorage.storeTests(wkspace, tests);
140137
this.disposeCancellationToken(CancellationTokenType.testDiscovery);
141138
sendTelemetryEvent(UNITTEST_DISCOVER, undefined, telementryProperties);
@@ -159,7 +156,7 @@ export abstract class BaseTestManager implements ITestManager {
159156
// tslint:disable-next-line:prefer-template
160157
this.outputChannel.appendLine(reason.toString());
161158
}
162-
const wkspace = workspace.getWorkspaceFolder(vscode.Uri.file(this.rootDirectory))!.uri;
159+
const wkspace = workspace.getWorkspaceFolder(Uri.file(this.rootDirectory))!.uri;
163160
this.testCollectionStorage.storeTests(wkspace, null);
164161
this.disposeCancellationToken(CancellationTokenType.testDiscovery);
165162
return Promise.reject(reason);
@@ -217,8 +214,9 @@ export abstract class BaseTestManager implements ITestManager {
217214
if (this.testDiscoveryCancellationToken && this.testDiscoveryCancellationToken.isCancellationRequested) {
218215
return Promise.reject<Tests>(reason);
219216
}
220-
displayTestErrorMessage('Errors in discovering tests, continuing with tests');
221-
return <Tests>{
217+
const testsHelper = this.serviceContainer.get<ITestsHelper>(ITestsHelper);
218+
testsHelper.displayTestErrorMessage('Errors in discovering tests, continuing with tests');
219+
return {
222220
rootTestFolders: [], testFiles: [], testFolders: [], testFunctions: [], testSuites: [],
223221
summary: { errors: 0, failures: 0, passed: 0, skipped: 0 }
224222
};
@@ -250,9 +248,9 @@ export abstract class BaseTestManager implements ITestManager {
250248
private createCancellationToken(tokenType: CancellationTokenType) {
251249
this.disposeCancellationToken(tokenType);
252250
if (tokenType === CancellationTokenType.testDiscovery) {
253-
this.testDiscoveryCancellationTokenSource = new vscode.CancellationTokenSource();
251+
this.testDiscoveryCancellationTokenSource = new CancellationTokenSource();
254252
} else {
255-
this.testRunnerCancellationTokenSource = new vscode.CancellationTokenSource();
253+
this.testRunnerCancellationTokenSource = new CancellationTokenSource();
256254
}
257255
}
258256
private disposeCancellationToken(tokenType: CancellationTokenType) {

‎src/client/unittests/common/managers/testConfigurationManager.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,26 @@
11
import * as path from 'path';
22
import { OutputChannel, QuickPickItem, Uri, window } from 'vscode';
33
import { createDeferred } from '../../../common/helpers';
4-
import { IInstaller, Product } from '../../../common/types';
4+
import { IInstaller, IOutputChannel, Product } from '../../../common/types';
55
import { getSubDirectories } from '../../../common/utils';
6+
import { IServiceContainer } from '../../../ioc/types';
7+
import { ITestConfigurationManager } from '../../types';
8+
import { TEST_OUTPUT_CHANNEL } from '../constants';
69
import { ITestConfigSettingsService, UnitTestProduct } from './../types';
710

8-
export abstract class TestConfigurationManager {
11+
export abstract class TestConfigurationManager implements ITestConfigurationManager {
12+
protected readonly outputChannel: OutputChannel;
13+
protected readonly installer: IInstaller;
14+
protected readonly testConfigSettingsService: ITestConfigSettingsService;
915
constructor(protected workspace: Uri,
1016
protected product: UnitTestProduct,
11-
protected readonly outputChannel: OutputChannel,
12-
protected installer: IInstaller,
13-
protected testConfigSettingsService: ITestConfigSettingsService) { }
14-
// tslint:disable-next-line:no-any
15-
public abstract configure(wkspace: Uri): Promise<any>;
17+
protected readonly serviceContainer: IServiceContainer) {
18+
this.outputChannel = serviceContainer.get<OutputChannel>(IOutputChannel, TEST_OUTPUT_CHANNEL);
19+
this.installer = serviceContainer.get<IInstaller>(IInstaller);
20+
this.testConfigSettingsService = serviceContainer.get<ITestConfigSettingsService>(ITestConfigSettingsService);
21+
}
22+
public abstract configure(wkspace: Uri): Promise<void>;
23+
public abstract requiresUserToConfigure(wkspace: Uri): Promise<boolean>;
1624
public async enable() {
1725
// Disable other test frameworks.
1826
const testProducsToDisable = [Product.pytest, Product.unittest, Product.nosetest]

‎src/client/unittests/common/services/configSettingService.ts‎

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,31 @@
1-
import { Uri, workspace, WorkspaceConfiguration } from 'vscode';
1+
import { inject, injectable } from 'inversify';
2+
import { Uri, WorkspaceConfiguration } from 'vscode';
3+
import { IWorkspaceService } from '../../../common/application/types';
24
import { Product } from '../../../common/types';
5+
import { IServiceContainer } from '../../../ioc/types';
36
import { ITestConfigSettingsService, UnitTestProduct } from './../types';
47

8+
@injectable()
59
export class TestConfigSettingsService implements ITestConfigSettingsService {
6-
private static getTestArgSetting(product: UnitTestProduct) {
10+
private readonly workspaceService: IWorkspaceService;
11+
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
12+
this.workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
13+
}
14+
public async updateTestArgs(testDirectory: string | Uri, product: UnitTestProduct, args: string[]) {
15+
const setting = this.getTestArgSetting(product);
16+
return this.updateSetting(testDirectory, setting, args);
17+
}
18+
19+
public async enable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
20+
const setting = this.getTestEnablingSetting(product);
21+
return this.updateSetting(testDirectory, setting, true);
22+
}
23+
24+
public async disable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
25+
const setting = this.getTestEnablingSetting(product);
26+
return this.updateSetting(testDirectory, setting, false);
27+
}
28+
private getTestArgSetting(product: UnitTestProduct) {
729
switch (product) {
830
case Product.unittest:
931
return 'unitTest.unittestArgs';
@@ -15,7 +37,7 @@ export class TestConfigSettingsService implements ITestConfigSettingsService {
1537
throw new Error('Invalid Test Product');
1638
}
1739
}
18-
private static getTestEnablingSetting(product: UnitTestProduct) {
40+
private getTestEnablingSetting(product: UnitTestProduct) {
1941
switch (product) {
2042
case Product.unittest:
2143
return 'unitTest.unittestEnabled';
@@ -28,36 +50,22 @@ export class TestConfigSettingsService implements ITestConfigSettingsService {
2850
}
2951
}
3052
// tslint:disable-next-line:no-any
31-
private static async updateSetting(testDirectory: string | Uri, setting: string, value: any) {
53+
private async updateSetting(testDirectory: string | Uri, setting: string, value: any) {
3254
let pythonConfig: WorkspaceConfiguration;
3355
const resource = typeof testDirectory === 'string' ? Uri.file(testDirectory) : testDirectory;
34-
if (!Array.isArray(workspace.workspaceFolders) || workspace.workspaceFolders.length === 0) {
35-
pythonConfig = workspace.getConfiguration('python');
36-
} else if (workspace.workspaceFolders.length === 1) {
37-
pythonConfig = workspace.getConfiguration('python', workspace.workspaceFolders[0].uri);
56+
if (!this.workspaceService.hasWorkspaceFolders) {
57+
pythonConfig = this.workspaceService.getConfiguration('python');
58+
} else if (this.workspaceService.workspaceFolders!.length === 1) {
59+
pythonConfig = this.workspaceService.getConfiguration('python', this.workspaceService.workspaceFolders![0].uri);
3860
} else {
39-
const workspaceFolder = workspace.getWorkspaceFolder(resource);
61+
const workspaceFolder = this.workspaceService.getWorkspaceFolder(resource);
4062
if (!workspaceFolder) {
4163
throw new Error(`Test directory does not belong to any workspace (${testDirectory})`);
4264
}
4365
// tslint:disable-next-line:no-non-null-assertion
44-
pythonConfig = workspace.getConfiguration('python', workspaceFolder!.uri);
66+
pythonConfig = this.workspaceService.getConfiguration('python', workspaceFolder!.uri);
4567
}
4668

4769
return pythonConfig.update(setting, value);
4870
}
49-
public async updateTestArgs(testDirectory: string | Uri, product: UnitTestProduct, args: string[]) {
50-
const setting = TestConfigSettingsService.getTestArgSetting(product);
51-
return TestConfigSettingsService.updateSetting(testDirectory, setting, args);
52-
}
53-
54-
public async enable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
55-
const setting = TestConfigSettingsService.getTestEnablingSetting(product);
56-
return TestConfigSettingsService.updateSetting(testDirectory, setting, true);
57-
}
58-
59-
public async disable(testDirectory: string | Uri, product: UnitTestProduct): Promise<void> {
60-
const setting = TestConfigSettingsService.getTestEnablingSetting(product);
61-
return TestConfigSettingsService.updateSetting(testDirectory, setting, false);
62-
}
6371
}

‎src/client/unittests/common/testUtils.ts‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import { inject, injectable, named } from 'inversify';
22
import * as path from 'path';
3-
import { commands, Uri, window, workspace } from 'vscode';
3+
import { Uri, window, workspace } from 'vscode';
4+
import { IApplicationShell, ICommandManager } from '../../common/application/types';
45
import * as constants from '../../common/constants';
56
import { IUnitTestSettings, Product } from '../../common/types';
7+
import { IServiceContainer } from '../../ioc/types';
68
import { CommandSource } from './constants';
79
import { TestFlatteningVisitor } from './testVisitors/flatteningVisitor';
810
import { ITestsHelper, ITestVisitor, TestFile, TestFolder, TestProvider, Tests, TestSettingsPropertyNames, TestsToRun, UnitTestProduct } from './types';
@@ -19,15 +21,6 @@ export async function selectTestWorkspace(): Promise<Uri | undefined> {
1921
}
2022
}
2123

22-
export function displayTestErrorMessage(message: string) {
23-
window.showErrorMessage(message, constants.Button_Text_Tests_View_Output).then(action => {
24-
if (action === constants.Button_Text_Tests_View_Output) {
25-
commands.executeCommand(constants.Commands.Tests_ViewOutput, undefined, CommandSource.ui);
26-
}
27-
});
28-
29-
}
30-
3124
export function extractBetweenDelimiters(content: string, startDelimiter: string, endDelimiter: string): string {
3225
content = content.substring(content.indexOf(startDelimiter) + startDelimiter.length);
3326
return content.substring(0, content.lastIndexOf(endDelimiter));
@@ -40,7 +33,13 @@ export function convertFileToPackage(filePath: string): string {
4033

4134
@injectable()
4235
export class TestsHelper implements ITestsHelper {
43-
constructor(@inject(ITestVisitor) @named('TestFlatteningVisitor') private flatteningVisitor: TestFlatteningVisitor) { }
36+
private readonly appShell: IApplicationShell;
37+
private readonly commandManager: ICommandManager;
38+
constructor(@inject(ITestVisitor) @named('TestFlatteningVisitor') private flatteningVisitor: TestFlatteningVisitor,
39+
@inject(IServiceContainer) serviceContainer: IServiceContainer) {
40+
this.appShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
41+
this.commandManager = serviceContainer.get<ICommandManager>(ICommandManager);
42+
}
4443
public parseProviderName(product: UnitTestProduct): TestProvider {
4544
switch (product) {
4645
case Product.nosetest: return 'nosetest';
@@ -165,4 +164,11 @@ export class TestsHelper implements ITestsHelper {
165164
// tslint:disable-next-line:no-object-literal-type-assertion
166165
return <TestsToRun>{ testFile: [{ name: name, nameToRun: name, functions: [], suites: [], xmlName: name, fullPath: '', time: 0 }] };
167166
}
167+
public displayTestErrorMessage(message: string) {
168+
this.appShell.showErrorMessage(message, constants.Button_Text_Tests_View_Output).then(action => {
169+
if (action === constants.Button_Text_Tests_View_Output) {
170+
this.commandManager.executeCommand(constants.Commands.Tests_ViewOutput, undefined, CommandSource.ui);
171+
}
172+
});
173+
}
168174
}

0 commit comments

Comments
 (0)