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

Add the proposed Port Attributes API for python testing #22245

Closed
wants to merge 6 commits into from

Conversation

eleanorjboyd
Copy link
Member

fixes #22021.

@alexr00, we are not able to provide a pattern for the extension like @paulacamargo25 was doing for debugpy and we are not able to define a port range. Is there something else I should be putting into the PortAttributesSelector?

@eleanorjboyd eleanorjboyd self-assigned this Oct 17, 2023
@eleanorjboyd eleanorjboyd added the feature-request Request for new features or functionality label Oct 17, 2023
@eleanorjboyd eleanorjboyd marked this pull request as draft October 17, 2023 21:59
@eleanorjboyd
Copy link
Member Author

@karthiknadig was package-lock supposed to be updated after I edited package.json for this change? Ran npm ci and nothing happened.

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

@alexr00, we are not able to provide a pattern for the extension like @paulacamargo25 was doing for debugpy and we are not able to define a port range. Is there something else I should be putting into the PortAttributesSelector?

Is there anything distinct about the port you care about here? We can expand the API to have other properties.

_attributes: { port: number; pid?: number; commandLine?: string },
_token: CancellationToken,
): ProviderResult<PortAttributes> {
return new PortAttributes(PortAutoForwardAction.Ignore);
Copy link
Member

Choose a reason for hiding this comment

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

This will cause every single port to be ignored, which essentially turns off auto port forwarding for all ports.
If you are trying to create a no-op implementation, then you should return undefined.

Copy link
Member

Choose a reason for hiding this comment

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

We create two ports while doing test discovery and running tests. One of them stays open till user closes VS Code, the other one is transient.

Both are created using net.Server : https://nodejs.org/api/net.html#serverlistenport-host-backlog-callback

Is there a way we can push this info into the provider? i.e., whenever we open a port could we do something like setPortAttributes(port: number|range, attribute: PortAutoForwardAction) for those ports, and when we close the server, we could resetPortAttributes(port: number|range).

Copy link
Member

Choose a reason for hiding this comment

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

The other option is, to return undefined until such a port is created, then we update internal state inside TestPortAttributesProvider with the updated list of ports that we hold active.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we have 2 problesm:

  1. You don't have enough information at registration time to provide a good PortAttributesSelector.
  2. You can't tell which port is yours based on the properties passed into providePortAttributes.

For problem 1 (and actually problem 2 too): are you able to simply wait to call registerPortAttributesProvider until you've started your process and know the port number? So long as you do the registerPortAttributesProvider in the same frame as you start the process, you will be registered in time. This would also solve problem 2, as you could pass your port number or pid into your PortAttributesProvider.

Copy link
Member Author

Choose a reason for hiding this comment

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

were able to call registerPortAttributesProvider at the time of creation of the port number so this should resolve that problem!

Copy link
Member

Choose a reason for hiding this comment

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

The setPortAttribute and resetPortAttribute functions look good for solving problem 2!

@eleanorjboyd eleanorjboyd requested a review from alexr00 October 19, 2023 20:12
@eleanorjboyd eleanorjboyd marked this pull request as ready for review October 19, 2023 20:14
@karthiknadig karthiknadig added the skip package*.json package.json and package-lock.json don't both need updating label Oct 19, 2023

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.

_attributes: { port: number; pid?: number; commandLine?: string },
_token: CancellationToken,
): ProviderResult<PortAttributes> {
return new PortAttributes(PortAutoForwardAction.Ignore);
Copy link
Member

Choose a reason for hiding this comment

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

The setPortAttribute and resetPortAttribute functions look good for solving problem 2!

@eleanorjboyd
Copy link
Member Author

closing as we are moving to using named pipes as a fix for some other issues and therefore will no longer be using ports that should be registered. Thanks!

const proc = spawn(file, args, spawnOptions);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality skip package*.json package.json and package-lock.json don't both need updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt the proposed Port Attributes API for python testing
3 participants