Skip to content

Add the proposed Port Attributes API for python testing #22245

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

Closed
wants to merge 6 commits into from
Closed
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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"quickPickSortByLabel",
"testObserver",
"quickPickItemTooltip",
"saveEditor"
"saveEditor",
"portsAttributes"
],
"author": {
"name": "Microsoft Corporation"
Expand Down
50 changes: 50 additions & 0 deletions src/client/testing/portAttributesProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* eslint-disable class-methods-use-this */
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import {
CancellationToken,
Disposable,
PortAttributes,
PortAttributesProvider,
PortAutoForwardAction,
ProviderResult,
workspace,
} from 'vscode';

class TestPortAttributesProvider implements PortAttributesProvider {
private knownPorts: number[] = [];

public providePortAttributes(
attributes: { port: number; pid?: number; commandLine?: string },
_token: CancellationToken,
): ProviderResult<PortAttributes> {
if (this.knownPorts.includes(attributes.port)) {
return new PortAttributes(PortAutoForwardAction.Ignore);
}
return undefined;
}

public setPortAttribute(port: number): void {
this.knownPorts.push(port);
}

public resetPortAttribute(port: number): void {
this.knownPorts = this.knownPorts.filter((p) => p !== port);
}
}

let provider: TestPortAttributesProvider | undefined;

export function registerTestPortAttributesProvider(disposables: Disposable[]): void {
provider = new TestPortAttributesProvider();
disposables.push(workspace.registerPortAttributesProvider({}, provider));
Copy link
Member

Choose a reason for hiding this comment

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

Passing {} is still not ideal for the selector. This means that your attributes provider will be called for each auto forwarded port. Is there really no commandPattern that you can specify that will at least filter out some ports?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of requiring that at least one value of the selector is set to a meaningful value and logging an error if it's not set properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@karthiknadig, thoughts on this? I am not sure about the specific of the commandPattern.

Copy link
Member

Choose a reason for hiding this comment

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

The port is created in the extension host, so the process itself is whatever process that extension host is running in. So we could specify the process id of the extension host.

}

export function setPortAttribute(port: number): void {
provider?.setPortAttribute(port);
}

export function resetPortAttribute(port: number): void {
provider?.resetPortAttribute(port);
}
17 changes: 15 additions & 2 deletions src/client/testing/testController/common/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
} from './utils';
import { createDeferred } from '../../../common/utils/async';
import { EnvironmentVariables } from '../../../api/types';
import { resetPortAttribute, setPortAttribute } from '../../portAttributesProvider';

export class PythonTestServer implements ITestServer, Disposable {
private _onDataReceived: EventEmitter<DataReceivedEvent> = new EventEmitter<DataReceivedEvent>();
Expand All @@ -36,6 +37,8 @@ export class PythonTestServer implements ITestServer, Disposable {

private ready: Promise<void>;

private port: number | undefined;

private _onRunDataReceived: EventEmitter<DataReceivedEvent> = new EventEmitter<DataReceivedEvent>();

private _onDiscoveryDataReceived: EventEmitter<DataReceivedEvent> = new EventEmitter<DataReceivedEvent>();
Expand Down Expand Up @@ -65,14 +68,20 @@ export class PythonTestServer implements ITestServer, Disposable {
});
this.ready = new Promise((resolve, _reject) => {
this.server.listen(undefined, 'localhost', () => {
this.port = (this.server.address() as net.AddressInfo).port;
setPortAttribute(this.port);
resolve();
});
});
this.server.on('error', (ex) => {
traceLog(`Error starting test server: ${ex}`);
});
this.server.on('close', () => {
traceLog('Test server closed.');
if (this.getPort()) {
resetPortAttribute(this.getPort());
traceLog('Test server closed, port attributes reset');
}
traceLog('Test server closed port attributes reset skipped.');
});
this.server.on('listening', () => {
traceLog('Test server listening.');
Expand Down Expand Up @@ -128,7 +137,11 @@ export class PythonTestServer implements ITestServer, Disposable {
}

public getPort(): number {
return (this.server.address() as net.AddressInfo).port;
if (this.port) {
return this.port;
}
traceError('Attempted to access port but it is undefined for the python testing server.');
return 45454;
}

public createUUID(): string {
Expand Down
12 changes: 11 additions & 1 deletion src/client/testing/testController/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
ITestResultResolver,
} from './types';
import { Deferred, createDeferred } from '../../../common/utils/async';
import { resetPortAttribute, setPortAttribute } from '../../portAttributesProvider';

export function fixLogLines(content: string): string {
const lines = content.split(/\r?\n/g);
Expand Down Expand Up @@ -166,6 +167,7 @@ export function pythonTestAdapterRewriteEnabled(serviceContainer: IServiceContai
}

export async function startTestIdServer(testIds: string[]): Promise<number> {
let port: number | undefined;
const startServer = (): Promise<number> =>
new Promise((resolve, reject) => {
const server = net.createServer((socket: net.Socket) => {
Expand All @@ -189,11 +191,19 @@ export async function startTestIdServer(testIds: string[]): Promise<number> {
socket.on('end', () => {
traceLog('Client disconnected');
});
socket.on('close', () => {
if (port) {
resetPortAttribute(port);
traceLog('Server closed, port attribute reset.');
}
traceLog('Server closed, port attribute reset skipped.');
});
});

server.listen(0, () => {
const { port } = server.address() as net.AddressInfo;
port = (server.address() as net.AddressInfo).port;
traceLog(`Server listening on port ${port}`);
setPortAttribute(port);
resolve(port);
});

Expand Down
4 changes: 4 additions & 0 deletions src/client/testing/testController/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// Licensed under the MIT License.

import { IExtensionSingleActivationService } from '../../activation/types';
import { IDisposableRegistry } from '../../common/types';
import { IServiceManager } from '../../ioc/types';
import { PYTEST_PROVIDER, UNITTEST_PROVIDER } from '../common/constants';
import { registerTestPortAttributesProvider } from '../portAttributesProvider';
import { TestDiscoveryHelper } from './common/discoveryHelper';
import { ITestFrameworkController, ITestDiscoveryHelper, ITestsRunner, ITestController } from './common/types';
import { PythonTestController } from './controller';
Expand All @@ -26,4 +28,6 @@ export function registerTestControllerTypes(serviceManager: IServiceManager): vo
serviceManager.addSingleton<ITestsRunner>(ITestsRunner, UnittestRunner, UNITTEST_PROVIDER);
serviceManager.addSingleton<ITestController>(ITestController, PythonTestController);
serviceManager.addBinding(ITestController, IExtensionSingleActivationService);
const disposables = serviceManager.get<IDisposableRegistry>(IDisposableRegistry);
registerTestPortAttributesProvider(disposables);
}
69 changes: 69 additions & 0 deletions src/test/testing/serviceRegistry.unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

'use strict';

import { instance, mock, verify } from 'ts-mockito';
import { registerTypes } from '../../client/activation/serviceRegistry';
import { IExtensionActivationService } from '../../client/activation/types';
import { ServiceManager } from '../../client/ioc/serviceManager';
import { IServiceManager } from '../../client/ioc/types';
import { TestConfigSettingsService } from '../../client/testing/common/configSettingService';
import { DebugLauncher } from '../../client/testing/common/debugLauncher';
import { TestRunner } from '../../client/testing/common/runner';
import { UnitTestSocketServer } from '../../client/testing/common/socketServer';
import { TestsHelper } from '../../client/testing/common/testUtils';
import {
ITestDebugLauncher,
ITestsHelper,
IUnitTestSocketServer,
ITestRunner,
ITestConfigurationService,
ITestConfigSettingsService,
ITestConfigurationManagerFactory,
} from '../../client/testing/common/types';
import { UnitTestConfigurationService } from '../../client/testing/configuration';
import { TestConfigurationManagerFactory } from '../../client/testing/configurationFactory';
import { TestingService, UnitTestManagementService } from '../../client/testing/main';
import { ITestingService } from '../../client/testing/types';

suite('Testing_Service_Registry', () => {
let serviceManager: IServiceManager;

setup(() => {
serviceManager = mock(ServiceManager);
});
test('Ensure services are registered', async () => {
registerTypes(instance(serviceManager));

verify(serviceManager.addSingleton<ITestDebugLauncher>(ITestDebugLauncher, DebugLauncher)).once();
verify(serviceManager.add<ITestsHelper>(ITestsHelper, TestsHelper)).once();
verify(serviceManager.add<IUnitTestSocketServer>(IUnitTestSocketServer, UnitTestSocketServer)).once();
verify(serviceManager.add<ITestRunner>(ITestRunner, TestRunner)).once();
verify(
serviceManager.addSingleton<ITestConfigurationService>(
ITestConfigurationService,
UnitTestConfigurationService,
),
).once();
verify(serviceManager.addSingleton<ITestingService>(ITestingService, TestingService)).once();
verify(
serviceManager.addSingleton<ITestConfigSettingsService>(
ITestConfigSettingsService,
TestConfigSettingsService,
),
).once();
verify(
serviceManager.addSingleton<ITestConfigurationManagerFactory>(
ITestConfigurationManagerFactory,
TestConfigurationManagerFactory,
),
).once();
verify(
serviceManager.addSingleton<IExtensionActivationService>(
IExtensionActivationService,
UnitTestManagementService,
),
).once();
});
});
105 changes: 105 additions & 0 deletions types/vscode.proposed.portsAttributes.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

declare module 'vscode' {
// https://github.com/microsoft/vscode/issues/115616 @alexr00

/**
* The action that should be taken when a port is discovered through automatic port forwarding discovery.
*/
export enum PortAutoForwardAction {
/**
* Notify the user that the port is being forwarded. This is the default action.
*/
Notify = 1,
/**
* Once the port is forwarded, open the user's web browser to the forwarded port.
*/
OpenBrowser = 2,
/**
* Once the port is forwarded, open the preview browser to the forwarded port.
*/
OpenPreview = 3,
/**
* Forward the port silently.
*/
Silent = 4,
/**
* Do not forward the port.
*/
Ignore = 5,
}

/**
* The attributes that a forwarded port can have.
*/
export class PortAttributes {
/**
* The action to be taken when this port is detected for auto forwarding.
*/
autoForwardAction: PortAutoForwardAction;

/**
* Creates a new PortAttributes object
* @param port the port number
* @param autoForwardAction the action to take when this port is detected
*/
constructor(autoForwardAction: PortAutoForwardAction);
}

/**
* A provider of port attributes. Port attributes are used to determine what action should be taken when a port is discovered.
*/
export interface PortAttributesProvider {
/**
* Provides attributes for the given port. For ports that your extension doesn't know about, simply
* return undefined. For example, if `providePortAttributes` is called with ports 3000 but your
* extension doesn't know anything about 3000 you should return undefined.
* @param port The port number of the port that attributes are being requested for.
* @param pid The pid of the process that is listening on the port. If the pid is unknown, undefined will be passed.
* @param commandLine The command line of the process that is listening on the port. If the command line is unknown, undefined will be passed.
* @param token A cancellation token that indicates the result is no longer needed.
*/
providePortAttributes(
attributes: { port: number; pid?: number; commandLine?: string },
token: CancellationToken,
): ProviderResult<PortAttributes>;
}

/**
* A selector that will be used to filter which {@link PortAttributesProvider} should be called for each port.
*/
export interface PortAttributesSelector {
/**
* Specifying a port range will cause your provider to only be called for ports within the range.
* The start is inclusive and the end is exclusive.
*/
portRange?: [number, number] | number;

/**
* Specifying a command pattern will cause your provider to only be called for processes whose command line matches the pattern.
*/
commandPattern?: RegExp;
}

export namespace workspace {
/**
* If your extension listens on ports, consider registering a PortAttributesProvider to provide information
* about the ports. For example, a debug extension may know about debug ports in it's debuggee. By providing
* this information with a PortAttributesProvider the extension can tell the editor that these ports should be
* ignored, since they don't need to be user facing.
*
* The results of the PortAttributesProvider are merged with the user setting `remote.portsAttributes`. If the values conflict, the user setting takes precedence.
*
* @param portSelector It is best practice to specify a port selector to avoid unnecessary calls to your provider.
* If you don't specify a port selector your provider will be called for every port, which will result in slower port forwarding for the user.
* @param provider The {@link PortAttributesProvider PortAttributesProvider}.
*/
export function registerPortAttributesProvider(
portSelector: PortAttributesSelector,
provider: PortAttributesProvider,
): Disposable;
}
}