Test API Feedback

[jrieken]

re #122208

This is a bit of a potpourri of API feedback regarding testing. Some are nit, some are about "firsts" 🥇, but also some bigger concerns 💭.

💭 Lacking "Run Options/Configs"

Two things have been modeled so far: "runnable" and "debuggable" (via TestItem#{runnable,debuggable} and TestRunRequest#debug). However, there is no notion of "run in safari" (playwright, vscode-selfhost use-case), "run with coverage", "run and profile" etc. As far as the UI is concerned we want to show commands and the flexible alternative is commands, menus, and context keys.

This concerned has been shelled out to #127096

💭 TestItem#id

A test item needs to have a unique identifier. It is not clear what this is used for because the id isn't "referenced" anywhere, instead the TestItem is used. E.g test results are collected by item instance ((TestRun#setState), not by id. The same for appendMessage etc. My understanding is that I can dispose a test item and re-create it with the same id and its results are kept (re-associated). Assuming that's correct, it should be modeled more explicit. Also, todays design makes me wonder why there isn't just TestItem.setState(runRequest, runResult), esp. in the presence of TestItem#invalidateResults?

💭 TestItems everywhere

TestItem is a recursive and lazy data structure that represents everything: file, suite, test etc. I understand the desire for lazy test containers, like files, but I doubt the usefulness for recursive laziness. Like, you can identify something as containing suites and tests and analyzing that something will take time but I doubt that the analysis itself will be lazy. My dummy extension does the following (and I bet non-sample extensions do the same):

• Each file is lazily parsed to detect tests. The file and each test is a managed object
• When typing occurs, I dispose all existing test objects and re-create new managed objects (just to dispose them on next type).

This feels cumbersome and I assume it is also why recursive disposal of test items was "invented". I would prefer a simple setter on a managed container object, like testContainer.items = [pojoTestItem1, pojoTestItem2]. I also doubt that any extension manages and updates existing items. Sample: when inserting a newline in a test file each item must get a new range property. Unless the test items are kept as AST-like incremental data structure it will all be "fake"; meaning some process (reflection, ast-traversal, execution) produces a set of new items and we now ask extensions to do the diffing. It is much more friendly when extensions can "throw new items over the fence" and VS Code does the diffing (e.g by test item id)

🥇 Firsts

• TestItem#dispose is recursive - I understand why this was done but it does violate the create/dispose balance.
• TestItem#children brings es6-maps & iterables into the API - I am a big fan of iterables and I know plenty of places in the API where they should be introduced. Still, I doubt if children is a justified "first case" and wonder what we loose if readonly TestItem[] is used. What would be the recommendation for furture map vs iterable vs array questions and what should happen to existing APIs?

TestController#root is not a test item

That smells like implementation detail making it to the API... I don't even wanna think about TestController#root.dispose() or TestRun.setState(ctrl.root, ...)

[connor4312]

My understanding is that I can dispose a test item and re-create it with the same id and its results are kept (re-associated)... Assuming that's correct, it should be modeled more explicit.

Yea. Or, in another common case that every extension hits, the user reloads the window. If we use object identity, we have no way to re-associated the results. In the initial API, before I had ID's, I used a "path of labels" but this was just a less well-specified ID, so I changed that.

Is modelling it more explicitly done by having a custom string type with documentation as you suggested in PM, or are you thinking of something more here?

Also, todays design makes me wonder why there isn't just TestItem.setState(runRequest, runResult), esp. in the presence of TestItem#invalidateResults?

To me it reads better as run.setState(item, runResult) to say that the item is given a result in the context of a certain test run. However we can cover this in #127096.

I would prefer a simple setter on a managed container object, like testContainer.items

This is somewhat tricky right now. In the current API, items have an unchanging parent from the moment of their creation. With an array of items, we give that control over to extensions, so items could have multiple parents, change parents, or duplicated within an array. These are errors we could check against and throw at runtime, but they're also errors which it's not possible to make in the current set of APIs.

(I think we could actually deal with changing parents pretty easily internally by deleting and re-creating the item, but allowing multiple parents would be a large change for dubious benefit.)

That said I see how items = [] is attractive as a way to clear all children, and it fixes the dispose pattern since you just remove items from the array to 'dispose' them.

TestController#root is not a test item

It is slightly special in that disposing it is invalid, but I think its utility outweighs its specialness:

• Setting the label, error state, debugability/runnability, and accessing its children are things that need to exist in some way. Setting its state is valid, too, which a test extension could do if global setup or teardown fails. If there is not a root test item, these need to be duplicated somehow in other APIs.
  • Edit: with Separate test runner API #127096 (comment) we would no longer need to indicate runnability or debugability, so that's slightly less that would need to be dealt with. The other cases, aside from accessing root children, are things that could be worked around or done in other ways. For example, you could create a fake "error" child if tests could not load, or set the state of all tests in the run to "failed" if global setup or teardown fails.
• Removing it means we need to introduce new semantics for how we indicate that all tests need to run in a request. Right now we simply say to extensions "run the tests we pass you and all their children."
• Having it there makes tree-transversal logic easier in general. Extension authors don't need to deal with having special cases for the 'not a root element' at the top of the tree.

[JustinGrote]

FWIW I'm much happier with the API now that "data" was moved out to a separate optional mapping implementation and all the generics can be pulled out.

I like that TestItem interface is now a "catchall" for any test tree item, and I can define any "metadata" I need for a test in my separately managed data object (or dynamically pulled, whatever I want). It should always be just that, a conduit/representation of an object in the UI, and not dictate to me how I run my tests or how I gather the data for my tests. If it were broken out by the vscode API into suites, groups, etc. then you are dictating to me how I organize my tests.

Also now that all discovery and run is funneled through the controller, I can choose if I want to define the logic then and there, or if I want to shunt the logic out to my little metadata object that represents the test, there's more freedom there.

I agree run.setstate makes more sense on the context, though it could be run.setTestState maybe to be a little more clear, but this could also be covered in the JSDoc.

I think the root is fine and having it be a test item means my interface or unions don't have to include some other "random" type.

I think #127096 is valid, however today that could be controlled by extension settings or an extension contributed "options" window with checkboxes and whatnot, though that interface should probably be standardized to the Test API, maybe a dropdown where additional configurations can be supplied by the extension similar to how the Debug dropdown works.

[connor4312]

I think #127096 is valid, however today that could be controlled by extension settings or an extension contributed "options" window with checkboxes and whatnot, though that interface should probably be standardized to the Test API, maybe a dropdown where additional configurations can be supplied by the extension similar to how the Debug dropdown works.

I added a proposal in here which you might like: #127096 (comment)

[jrieken]

This is somewhat tricky right now. In the current API, items have an unchanging parent from the moment of their creation. With an array of items, we give that control over to extensions, so items could have multiple parents,

In a way we wouldn't care too much. For us, only the assignment to items would count. It is the place for minimal validation and from then on we consider the data "dead" because we have taken the necessary pieces and applied them to the UI. It would be enough that test items have children pointer and that they are eventually contained in a "container". In "pseudo API"

// used to represent a "snapshot" like suites and tests for a certain document version
interface TestItem {
  id: string;
  location: Uri | Location;
  label: string;
  
  children?: TestItem[];

  // more...
}

// managed object with lifecycle, like a file
interface TestContainer {

  dispose(): void;

  readonly id: string;
  readonly uri: Uri | undefined;

  // get/set to update test items
  items: TestItem[]

  // more...
}

function createTestContainer(id: string): TestContainer

interface TestController {
  // get/set
  tests: TestContainer[]
}

For me the appeal is in the simplicity for extensions: a test container would represent a "source of truth" like a file or live document and extensions can simply extract data from them and set it onto the corresponding container. No need for cleanup or object updates

[jrieken]

Yea. Or, in another common case that every extension hits, the user reloads the window. [...] Is modelling it more explicitly done by having a custom string type with documentation as you suggested in PM, or are you thinking of something more here?

I don't actually know. There is one part that makes me think test id are like uris and then they would deserve a type. And that type should be used. However, they are also different because they only allow for identification, but not locating, of items. Looking at the use-cases you have listed I wonder how mandatory the id is or if something like a version id is missing (this touches on invalidateResults). E.g when a test changes, like adding/removing assertions, should its id change so that old results are dropped? At runtime there is the invalidate-function but what happens after reload? As a side note: as a user, I am often puzzled that I have results before running any test.

[jrieken]

To me it reads better as run.setState(item, runResult) to say that the item is given a result in the context of a certain test run.

I like that too but I don't like invalidateResults because it somehow breaks the nice decoupling. Maybe, the aforementioned version id would help