Separate test runner API

[connor4312]

From discussion offline:

@jrieken: I still haven’t started nit-mode but things are looking much better. One thing that occurred to me is about createTestRun. Its comment says that a label can be used to distinguish different platforms on which the test run. I wonder if that’s enough? Not all tests run on all platforms (like for us only things from /browser/ or below run in safari) and it doesn’t offer a way for users to say “Re-run/run in Firefox”. Is it that a test run must always occur on all platforms? Should TestItem.runnable list “test runners”? What’s your take on this? Has this been already discussed and decided?
@connor4312: This is something the playwright team brought up as well, more around wanting to allow users to select what browsers they want to run on. Perhaps we can introduce a new "configurations" concept, where there is a set-like object of them on controllers. The UI could provide a way to run subsets of configurations (or the entire list of configurations) which could be passed in the test run request. Wrt items that only run on one platform, I think the way to do those right now would be marking them as "skipped" when run on platforms where they aren't valid. Not sure I want to add a bunch of complexity to the current "runnable" boolean...

Perhaps this could be something like this:

export class TestRunner<T = any> {
	name: string;
	runHandler: (request: TestRunRequest<T>) => void;
	constructor(name: string, runHandler: (request: TestRunRequest<T>) => void);
}

/**
 * Interface to discover and execute tests.
 */
export interface TestController<T = any> {
	registerTestRunner(runner: TestRunner<T>): Disposable;

With explicit handlers, this calls into question the need to do the magic matching on the request in createTestRun. However there is still the case of createTestRun being used to publish test runs without a run actually being executed in the vscode UI.

[connor4312]

So I realized what I want to express in the UI is not runners, but configurations. For example, in playwright you might run tests in edge, run tests in all browsers, or do this but with coverage, and so on. This leads me to a different design, where explicitly allow extensions to create configurations:

// Todo: this is basically the same as the TaskGroup, which is a class that
// allows custom groups to be created. However I don't anticipate having any
// UI for that, so enum for now?
export enum TestRunConfigurationGroup {
	Run = 1,
	Debug = 2,
	Coverage = 3,
}


export interface TestRunConfiguration {
	/**
	 * Configures where this configuration is grouped in the UI. If there
	 * are no configurations for a group, it will not be available in the UI.
	 */
	readonly group: TestRunConfigurationGroup;
	
	/**
	 * Label shown to the user in the UI.
	 */
	label: string;

	/**
	 * Controls whether this configuration is the default action that will
	 * be taken when its group is actions. For example, if the user clicks
	 * the generic "run all" button, then the default configuration for
	 * {@link TestRunConfigurationGroup.Run} will be executed.
	 */
	isDefault: boolean;

	/**
	 * If this method is present a configuration gear will be present in the
	 * UI, and this method will be invoked when it's clicked. When called,
	 * you can take other editor actions, such as showing a quick pick or
	 * opening a configuration file.
	 */
	configureHandler?: () => void;

	/** <moved from the TestController> */
	runHandler: (req: TestRunRequest, token: CancellationToken) => void;

	/**
	 * Deletes the run configuration.
	 */
	dispose(): void;
}

export interface TestController {
	createRunConfiguration(label: string, group: TestRunConfigurationGroup, runHandler: (req: TestRunRequest) => void): TestRunConfiguration;

In this case playwright could, for example, have a "run all" configuration, which creates multiple runs when invoked, in addition to individual browsers. The TestRunRequest can then omit the debug boolean, since it would use these configurations instead.

And we can also remove debuggable and runnable from the TestItem. That introduces a gap for Go/Python "subtests" where children are not runnable, but I think in these cases it would be acceptable for them to automatically run the parent of the requested subtest. Debugability would be indicated by the presence of any Debug-group configuration.

With this we would can keep the the current execution model unchanged, aside from the adjustment to the TestRunRequest and location of the runHandler. I think this approach solves a lot of problems.

[jrieken]

So I realized what I want to express in the UI is not runners, but configurations. For example, in playwright you might run tests in edge, run tests in all browsers, or do this but with coverage, and so on

I like that thinking. I'd say the controller is the runner (without suggesting to change the name) and configs are all different ways to make the runner "do stuff"

[connor4312]

@connectdotz mentioned auto run, which is a useful UI utility but is a bit of a sore spot, since currently VS Code core implements auto run but extensions can potentially do it better themselves.

I suggest an additional method on the configuration:

export interface TestRunConfiguration {
    // called with true when auto run is requested, false when it's turned off.
    watchHandler?: (enabled: boolean) => void;

[connectdotz]

@connor4312 if we are going to use the configuration handlers as an indicator for the UI, a few questions:

1. in multi-root projects, they might have different configurations per workspace folder, how should these be handled? Right now I used the TestItem level runnable to control on the item level, if we are going to roll them up to the configuration level per extension, we might end up dealing with lots of "sorry this action is not supported for these tests" post messages or unnecessary SKIPPED state that blocked the real run state...
2. For multi-root with mixed autoRun situations, how should the UI experience be like? Do we keep the button active and when invoking the onDidChangeAutoRun we show warning/error messages? How will the user know which workspace is under autoRun watch if the autoRun switch on top does not necessarily reflect the true state?

I don't object to the configuration-driven run API, but maybe we could still keep the item level config so they can express the micro-state/action like runnable, debuggable, and maybe even autoRun state?

[connor4312]

For multiple workspace folders, I think a decent approach is to use multiple test controllers.

I think we can figure out some good UI for the mixed autorun scenario. For example, if there are multiple controllers we might show the auto run button on items in the tree view instead of the menu bar. Or keep it in the menu bar but have some visual indicator of which controllers are currently autorunning.

[Bilalh]

And we can also remove debuggable and runnable from the TestItem. That introduces a gap for Go/Python "subtests" where children are not runnable, but I think in these cases it would be acceptable for them to automatically run the parent of the requested subtest. Debugability would be indicated by the presence of any Debug-group configuration.

Would pytest's parametrise which creates nested tests be a "subtest" https://docs.pytest.org/en/6.2.x/parametrize.html. You run/debug any individual part of a parametrised test in the vscode python extension, so if that would not be possible with the builtin api that would be an unfortunate regression.

[connor4312]

Would pytest's parametrise which creates nested tests be a "subtest"

Yes

if that would not be possible with the builtin api that would be an unfortunate regression.

It's cool that you can do that, and that would certainly be possible. What I was saying is that, if you couldn't run an individual subtest (e.g. Go's subtests) then you just have to run the parent.

[jdneo]

@connor4312 I have a question about the test configuration part.

• Does that mean we will have some files that persists these configuration?
• configureHandler?: () => void; Is there any limitation for this API? Or is it free for the extension to have whatever logic inside it?

[connor4312]

Does that mean we will have some files that persists these configuration?

If you want to, you can. VS Code doesn't manage these configurations, the extension does.

Or is it free for the extension to have whatever logic inside it?

You can do whatever you want to here. Show quickpicks, open webviews, open files, etc.

[connectdotz]

@connor4312 thanks for the reply:

For multiple workspace folders, I think a decent approach is to use multiple test controllers.

hmmm... interesting, I thought the test architecture is based on the single-controller per extension... oh well, if supporting multiple controllers will not require too much change, then push the controller down to the workspace level will probably be sufficient for our use case, as far as the granularity of the configuration goes...

I think we can figure out some good UI for the mixed autorun scenario.

cool. I hope it also applies to the run/debug config, i.e. each workspace should be able to configure its own run/debug as well. On the other hand, If we go this route, do we lose the ability to have extension-wide configs? Because even for multi-root, it is still handy to run all tests for all folders, for example, instead of clicking on each folder's run menu.

More about autoRun, I think somebody might have raised a similar issue: in our world, a test needs to be auto-run not only when the test file changed but also when the related source file(s) changed. Current autoRun implementation is handy but really only covers half of the story. To get autoRun fully working, it might require some more work. Not sure what is the test API rollout plan, is it going to be incremental or it won't be released until coverage, autoRun all got finalized?

Another question regarding createRunConfiguration: can the extension create multiple configs for a given TestRunConfigurationGroup? such as for TestRunConfigurationGroup.Run, could one create "run-all", "run-failed", "run-with-coverage" etc ?

[connor4312]

Because even for multi-root, it is still handy to run all tests for all folders, for example, instead of clicking on each folder's run menu.

That'll be the default behavior of the "run all" button (running all isDefault run-group configs).

Current autoRun implementation is handy but really only covers half of the story. To get autoRun fully working, it might require some more work. Not sure what is the test API rollout plan, is it going to be incremental or it won't be released until coverage, autoRun all got finalized?

Following some discussion this morning I've modified the suggestion slightly: #127096 (comment) If the handler is provided, then auto run is entirely in control of the extension.

Although even today, you can still call invalidate on test items when the related source changes.

can the extension create multiple configs for a given TestRunConfigurationGroup? such as for TestRunConfigurationGroup.Run, could one create "run-all", "run-failed", "run-with-coverage" etc ?

You can, and this is the intention of the API, though those are not good examples: "run failed' is already provided by VS Code core (default keybinding is ctrl+; + f) and "run with coverage" will be. It's useful if you have multiple ways to run tests, for example tests for a JavaScript package may be able to execute in Chrome, Firefox, and Edge, which would be individual run configurations.

[godcodehunter]

So I realized what I want to express in the UI is not runners, but configurations. For example, in playwright you might run tests in edge, run tests in all browsers, or do this but with coverage, and so on. This leads me to a different design, where explicitly allow extensions to create configurations:

// Todo: this is basically the same as the TaskGroup, which is a class that
// allows custom groups to be created. However I don't anticipate having any
// UI for that, so enum for now?
export enum TestRunConfigurationGroup {
	Run = 1,
	Debug = 2,
	Coverage = 3,
}


export interface TestRunConfiguration {
	/**
	 * Configures where this configuration is grouped in the UI. If there
	 * are no configurations for a group, it will not be available in the UI.
	 */
	readonly group: TestRunConfigurationGroup;
	
	/**
	 * Label shown to the user in the UI.
	 */
	label: string;

	/**
	 * Controls whether this configuration is the default action that will
	 * be taken when its group is actions. For example, if the user clicks
	 * the generic "run all" button, then the default configuration for
	 * {@link TestRunConfigurationGroup.Run} will be executed.
	 */
	isDefault: boolean;

	/**
	 * If this method is present a configuration gear will be present in the
	 * UI, and this method will be invoked when it's clicked. When called,
	 * you can take other editor actions, such as showing a quick pick or
	 * opening a configuration file.
	 */
	configureHandler?: () => void;

	/** <moved from the TestController> */
	runHandler: (req: TestRunRequest, token: CancellationToken) => void;

	/**
	 * Deletes the run configuration.
	 */
	dispose(): void;
}

export interface TestController {
	createRunConfiguration(label: string, group: TestRunConfigurationGroup, runHandler: (req: TestRunRequest) => void): TestRunConfiguration;

In this case playwright could, for example, have a "run all" configuration, which creates multiple runs when invoked, in addition to individual browsers. The TestRunRequest can then omit the debug boolean, since it would use these configurations instead.

And we can also remove debuggable and runnable from the TestItem. That introduces a gap for Go/Python "subtests" where children are not runnable, but I think in these cases it would be acceptable for them to automatically run the parent of the requested subtest. Debugability would be indicated by the presence of any Debug-group configuration.

With this we would can keep the the current execution model unchanged, aside from the adjustment to the TestRunRequest and location of the runHandler. I think this approach solves a lot of problems.

So what about creating a custom profile group?

interface RunProfileGroup {
    id: string,
    iconPath?: string | Uri | { light: string | Uri; dark: string | Uri } | ThemeIcon;
    tooltip?: string | MarkdownString | undefined;
    ...
    constructor(id, iconPath, ...)
}

It still looks necessary to implement autorun. Also, seems it should support toggling

[connor4312]

Custom run groups are not supported at this time. If you have a use case for them, please open an issue with details 🙂

Autorun should be coming this or next month. I still need to think about how best to implement it -- during the "proposed" period we had an implementation of it, but it was flawed and not something I was ready to finalize.

[godcodehunter]

Autorun should be coming this or next month. I still need to think about how best to implement it -- during the "proposed" period we had an implementation of it, but it was flawed and not something I was ready to finalize.

Why you just not use treeDataProvider as pillars for test API. I want say that current API hide data structure, unlazy and in general, the presence of two entities and approaches without serious motivation violates the principle of Okame's Razor

[connor4312]

Please see the discussion in #107467 for reasoning on why the API is designed this way.

[godcodehunter]

I've seen this discussion. Could you name 2-3 main reasons why treeDataProvider was not used as pillars for that API.

[godcodehunter]

Please see the discussion in #107467 for reasoning on why the API is designed this way.

Also, really strange why TestItem have uri and range as a separate field instead represent just like type sum like location?: Uri | Location.

[connor4312]

Let me know if you have any specific usage questions or scenarios where the API doesn't meet your needs. This API has been in development for close to a year, and now that it is in stable we cannot make breaking changes, so reopening past discussions is not useful. Thanks 🙂

[godcodehunter]

Let me know if you have any specific usage questions or scenarios where the API doesn't meet your needs. This API has been in development for close to a year, and now that it is in stable we cannot make breaking changes, so reopening past discussions is not useful. Thanks 🙂

As I said, autorun and asynchronous branch expansion. But are you planning on making breaking changes sometime? For me, it is better to conduct discussions of problems and oddities constantly, so that by gaining a significant mass they can be introduced, even at next year. I'm just trying to understand the motivation behind what has already been done. Perhaps this is not the best place, but there is no one from the team in the Gitter chat.

[connor4312]

We do not make breaking changes to stabilized APIs, though there is of course still plenty of room to make changes in a non-breaking way.

Please open separate issues with details around things you want to discuss! The bot will eventually lock this issue and open ones are easier to track the status of. For specific info around why the tree is built the way it is, check this issue and specifically this comment discussing the TreeDataProvider: #115089 (comment)