Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getting display name and version when using windows registry #1697

Merged
merged 7 commits into from
May 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,22 @@
],
"preLaunchTask": "Compile"
},
{
"name": "Debug Unit Tests",
"type": "node",
"request": "launch",
"program": "${workspaceFolder}/out/test/unittests.js",
"stopOnEntry": false,
"sourceMaps": true,
"args": [
"timeout=60000",
"grep="
],
"outFiles": [
"${workspaceFolder}/out/**/*.js"
],
"preLaunchTask": "Compile"
},
{
"name": "Launch Multiroot Tests",
"type": "extensionHost",
Expand Down Expand Up @@ -134,4 +150,4 @@
]
}
]
}
}
11 changes: 10 additions & 1 deletion .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@
"isDefault": true
}
},
{
"label": "Run Unit Tests",
"type": "npm",
"script": "test:unittests",
"group": {
"kind": "test",
"isDefault": true
}
},
{
"label": "Hygiene",
"type": "gulp",
Expand Down Expand Up @@ -105,4 +114,4 @@
}
}
]
}
}
9 changes: 8 additions & 1 deletion gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -502,5 +502,12 @@ exports.hygiene = hygiene;

// this allows us to run hygiene as a git pre-commit hook.
if (require.main === module) {
run({ exitOnError: true, mode: 'staged' });
const result = run({ exitOnError: true, mode: 'staged' });
// Run unit tests and ensure they pass as well.
if (result && result.on) {
result.on('end', () => {
const main = require('./out/test/unittests');
main.runTests();
})
}
}
1 change: 1 addition & 0 deletions news/3 Code Health/1703.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Run unit tests as a pre-commit hook.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1854,6 +1854,7 @@
"compile": "tsc -watch -p ./",
"postinstall": "node ./node_modules/vscode/bin/install",
"test": "node ./out/test/standardTest.js && node ./out/test/multiRootTest.js",
"test:unittests": "node ./out/test/unittests.js",
"testDebugger": "node ./out/test/debuggerTest.js",
"testSingleWorkspace": "node ./out/test/standardTest.js",
"testMultiWorkspace": "node ./out/test/multiRootTest.js",
Expand Down
6 changes: 5 additions & 1 deletion src/client/common/platform/pathUtils.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { inject, injectable } from 'inversify';
import * as path from 'path';
import { IPathUtils, IsWindows } from '../types';
import { NON_WINDOWS_PATH_VARIABLE_NAME, WINDOWS_PATH_VARIABLE_NAME } from './constants';

// TO DO: Deprecate in favor of IPlatformService
@injectable()
export class PathUtils implements IPathUtils {
constructor( @inject(IsWindows) private isWindows: boolean) { }
constructor(@inject(IsWindows) private isWindows: boolean) { }
public getPathVariableName() {
return this.isWindows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
}
public basename(pathValue: string, ext?: string): string{
Copy link

@MikhailArkhipov MikhailArkhipov May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically it is not necessary to wrap over Path since behavior does not change across systems and it does not operate over 'real' things like actual files. We prob never going to mock it. So why not use path directly? We could eventually move getPathVariableName to IPlatform and drop this service althogether.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did want to mock this for one of the tests. I was relying on the tests to run on AppVeyor.
Mocking this makes it possible to run the tests on Travis as well, and not have to rely just on AppVeyor to run tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm OK. Is behavior of basename is different across OS? Or did you mean path variable casing?

Copy link
Author

@DonJayamanne DonJayamanne May 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basename is different across OS, as path separator is different, it is either / or \

return path.basename(pathValue, ext);
}
}
1 change: 1 addition & 0 deletions src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const IPathUtils = Symbol('IPathUtils');

export interface IPathUtils {
getPathVariableName(): 'Path' | 'PATH';
basename(pathValue: string, ext?: string): string;
}

export const ICurrentProcess = Symbol('ICurrentProcess');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as _ from 'lodash';
import * as path from 'path';
import { Uri } from 'vscode';
import { Architecture, IRegistry, RegistryHive } from '../../../common/platform/types';
import { Is64Bit } from '../../../common/types';
import { IPathUtils, Is64Bit } from '../../../common/types';
import { IServiceContainer } from '../../../ioc/types';
import { IInterpreterHelper, InterpreterType, PythonInterpreter } from '../../contracts';
import { CacheableLocatorService } from './cacheableLocatorService';
Expand All @@ -27,10 +27,12 @@ type CompanyInterpreter = {

@injectable()
export class WindowsRegistryService extends CacheableLocatorService {
private readonly pathUtils: IPathUtils;
constructor(@inject(IRegistry) private registry: IRegistry,
@inject(Is64Bit) private is64Bit: boolean,
@inject(IServiceContainer) serviceContainer: IServiceContainer) {
super('WindowsRegistryService', serviceContainer);
this.pathUtils = serviceContainer.get<IPathUtils>(IPathUtils);
}
// tslint:disable-next-line:no-empty
public dispose() { }
Expand Down Expand Up @@ -72,7 +74,7 @@ export class WindowsRegistryService extends CacheableLocatorService {
private async getCompanies(hive: RegistryHive, arch?: Architecture): Promise<CompanyInterpreter[]> {
return this.registry.getKeys('\\Software\\Python', hive, arch)
.then(companyKeys => companyKeys
.filter(companyKey => CompaniesToIgnore.indexOf(path.basename(companyKey).toUpperCase()) === -1)
.filter(companyKey => CompaniesToIgnore.indexOf(this.pathUtils.basename(companyKey).toUpperCase()) === -1)
.map(companyKey => {
return { companyKey, hive, arch };
}));
Expand Down Expand Up @@ -125,7 +127,7 @@ export class WindowsRegistryService extends CacheableLocatorService {
if (!details) {
return;
}
const version = interpreterInfo.version ? path.basename(interpreterInfo.version) : details.version;
const version = interpreterInfo.version ? this.pathUtils.basename(interpreterInfo.version) : this.pathUtils.basename(tagKey);
// tslint:disable-next-line:prefer-type-cast no-object-literal-type-assertion
return {
...(details as PythonInterpreter),
Expand Down Expand Up @@ -155,7 +157,7 @@ export class WindowsRegistryService extends CacheableLocatorService {
if (displayName && displayName.length > 0) {
return displayName;
}
const company = path.basename(companyKey);
const company = this.pathUtils.basename(companyKey);
return company.toUpperCase() === PythonCoreComany ? PythonCoreCompanyDisplayName : company;
}
}
27 changes: 17 additions & 10 deletions src/client/unittests/common/managers/testConfigurationManager.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,46 @@
import * as path from 'path';
import * as vscode from 'vscode';
import { Uri } from 'vscode';
import { OutputChannel, QuickPickItem, Uri, window } from 'vscode';
import { createDeferred } from '../../../common/helpers';
import { IInstaller } from '../../../common/types';
import { IInstaller, Product } from '../../../common/types';
import { getSubDirectories } from '../../../common/utils';
import { ITestConfigSettingsService, UnitTestProduct } from './../types';

export abstract class TestConfigurationManager {
constructor(protected workspace: Uri,
protected product: UnitTestProduct,
protected readonly outputChannel: vscode.OutputChannel,
protected readonly outputChannel: OutputChannel,
protected installer: IInstaller,
protected testConfigSettingsService: ITestConfigSettingsService) { }
// tslint:disable-next-line:no-any
public abstract configure(wkspace: Uri): Promise<any>;
public async enable() {
// Disable other test frameworks.
const testProducsToDisable = [Product.pytest, Product.unittest, Product.nosetest]
.filter(item => item !== this.product) as UnitTestProduct[];

for (const prod of testProducsToDisable) {
await this.testConfigSettingsService.disable(this.workspace, prod);
}

return this.testConfigSettingsService.enable(this.workspace, this.product);
}
// tslint:disable-next-line:no-any
public async disable() {
return this.testConfigSettingsService.enable(this.workspace, this.product);
}
protected selectTestDir(rootDir: string, subDirs: string[], customOptions: vscode.QuickPickItem[] = []): Promise<string> {
protected selectTestDir(rootDir: string, subDirs: string[], customOptions: QuickPickItem[] = []): Promise<string> {
const options = {
matchOnDescription: true,
matchOnDetail: true,
placeHolder: 'Select the directory containing the unit tests'
};
let items: vscode.QuickPickItem[] = subDirs
let items: QuickPickItem[] = subDirs
.map(dir => {
const dirName = path.relative(rootDir, dir);
if (dirName.indexOf('.') === 0) {
return;
}
return <vscode.QuickPickItem>{
return {
label: dirName,
description: ''
};
Expand All @@ -44,7 +51,7 @@ export abstract class TestConfigurationManager {
items = [{ label: '.', description: 'Root directory' }, ...items];
items = customOptions.concat(items);
const def = createDeferred<string>();
vscode.window.showQuickPick(items, options).then(item => {
window.showQuickPick(items, options).then(item => {
if (!item) {
return def.resolve();
}
Expand All @@ -61,7 +68,7 @@ export abstract class TestConfigurationManager {
matchOnDetail: true,
placeHolder: 'Select the pattern to identify test files'
};
const items: vscode.QuickPickItem[] = [
const items: QuickPickItem[] = [
{ label: '*test.py', description: 'Python Files ending with \'test\'' },
{ label: '*_test.py', description: 'Python Files ending with \'_test\'' },
{ label: 'test*.py', description: 'Python Files begining with \'test\'' },
Expand All @@ -70,7 +77,7 @@ export abstract class TestConfigurationManager {
];

const def = createDeferred<string>();
vscode.window.showQuickPick(items, options).then(item => {
window.showQuickPick(items, options).then(item => {
if (!item) {
return def.resolve();
}
Expand Down
Loading