Testing API finalization

[connor4312]

Separating from #107467 to have a focused issue. These APIs will be finalized (ctrl+f for 122208 if this moves from additional changes in the file): https://github.com/microsoft/vscode/blob/main/src/vs/vscode.proposed.d.ts#L1972

[matepek]

Feature request:

• tags: TestItem { tags?: string[] }

How I imagine:

• Testsuite1
  • Test1 [tag1]
  • Test2 [tag1] [tag2]

The actual: ${label} ${tags.map(t=> '['+t+']').join(' ')}
The tag part can be the less prominent part (smaller font)

[connor4312]

Test items have a description for this purpose (though it is not curently shown in UI)

[matepek]

When TestItem is disposed the data might wanna be disposed too.
It can be managed in another way but it just seems convenient in case someone would like to create a disposable data.

[matepek]

Test items have a description for this purpose (though it is not curently shown in UI)

Okay thats something but one point users might wanna do filtering by tags. In that case if I only use it as a string in the description:

• search should include the description
• search should treat space as .* because item with [a] [b] [c] should be a part of the result if we search for [a] [c].

Well I didn't go to details. I'm just hoping it's clear. Let me know if you need clarification.

It would be a nice feature I believe.

Question: when can I modify the error, label, description, etc fields? Will they be reflected automatically?

[matepek]

Does the readonly uri: Uri; in TestItem has to be readonly? Is there any advantage in vscode side?

[connor4312]

Question: when can I modify the error, label, description, etc fields? Will they be reflected automatically?

Yes

Does the readonly uri: Uri; in TestItem has to be readonly? Is there any advantage in vscode side?

It simplifies things. We didn't find many use cases where the URI would change without tests being recreated/re-read

[matepek]

Does the readonly uri: Uri; in TestItem has to be readonly? Is there any advantage in vscode side?

It simplifies things. We didn't find many use cases where the URI would change without tests being recreated/re-read

Here is one but I would say pretty rear: User moves tests from test1.cpp to test2.cpp which are bundled to the same executable. after recompilation the URI will change of course. With the current API I have to check the URI too and if there is a need for update I have to dispose and recreate. It's not a biggie though just for the record.

How about my previous question: What if I don't have URI? What should I pass? Null?

[connor4312]

URIs are required on all tests. If you don't have a specific file, you can pass in the URI to a relevant directory instead.

[matepek]

URIs are required on all tests. If you don't have a specific file, you can pass in the URI to a relevant directory instead.

Even if the only relevant I can figure out is the workspaceFolder itself?

How will that effect the UX?

[connor4312]

Well, if you have no location for a test we will not be able to show things like editor decorations, "go to test" will not work, etc -- essentially you'll only get the overview of tests in the test explorer view, and little else. The experience is significantly less useful without having URIs for tests. This is not a common scenario and not something that I want to go out of the way to design for.

[matepek]

Okay. As I said it is a really valid situation/use-case when I won't have URI.
Another example:

• executable without source file at all.
• google test v < 1.8 doesn't provide the info
• ...

[connor4312]

The URI is now optional

[jdneo]

Some feedback about showing the test message when I was trying the new API.

In the hover box, looks like the test message is not using all the horizontal spaces. Meanwhile, if the test message is a Markdown string, it will show as [Object Object] inline.

BTW, by saying finalization, does that mean the listed API will go to stable VS Code?

[connor4312]

if the test message is a Markdown string, it will show as [Object Object] inline.

Thanks, fixed

In the hover box, looks like the test message is not using all the horizontal spaces

This uses the default VS Code hover, please open a separate issue for that!

BTW, by saying finalization, does that mean the listed API will go to stable VS Code?

Yes

[jdneo]

Created #123927.

BTW, where can I see the test execution duration in the UI? (Maybe I missed somewhere)

[connor4312]

Currently test execution duration isn't shown, but it will be soon.

[connor4312]

Duration will be shown in the test explorer view as of the next Insiders

[razzeee]

Will this make it into 1.57? I'm assuming we could start shipping our extensions with https://marketplace.visualstudio.com/items?itemName=ms-vscode.test-adapter-converter then?

[JustinGrote]

Was able to update without much issue, but this runhandler conversion hurts my brain so much.

microsoft/vscode-test-adapter-converter@197021c?branch=197021c010f01d93ef69eb919ecc0355f75f6757&diff=split

Did this instead:

    testController.createRunProfile('Run',TestRunProfileGroup.Run, this.runHandler, true)
    testController.createRunProfile('Debug',TestRunProfileGroup.Debug, this.debugHandler, true)
...


    private async runHandler(request: TestRunRequest) {
        this.testHandler(request, false)
    }
    private async debugHandler(request: TestRunRequest) {
        this.testHandler(request, true)
    }

    /** The test controller API calls this when tests are requested to run in the UI. It handles both runs and debugging */
    private async testHandler(request: TestRunRequest, debug: boolean) {
...

Also this API is so much cleaner now, thank you!

const testWatcher = workspace.createFileSystemWatcher(pattern)
const tests = this.testController.items
testWatcher.onDidCreate(uri => tests.add(TestFile.getOrCreate(testController, uri)))
testWatcher.onDidDelete(uri => tests.delete(uri.toString()))
testWatcher.onDidChange(uri => this.testController.resolveChildrenHandler!(TestFile.getOrCreate(testController, uri)))

[JustinGrote]

Also having trouble testing, the proposed.d.ts don't seem to be in this insiders, tried a refresh, guess I'll test tomorrow.

[connor4312]

@JustinGrote Insiders doesn't get released on Friday nights, it'll be out Monday morning.

I did that just to force you to learn more js/ts 😉 It provided a minimal conversion, though higher order functions are more common in client-side code.

---

Thanks for the feedback on runnability. I'm considering something like this, but will continue to think it over:

One solution to this that I think could work decently well is having a run configuration be able to define one (or more) "tag" strings, and allow tests items to have an array of tag strings. For any configuration where tags are defined, the editor would only apply that to test items that have the same tag. We had a request for tagging before as an organizational tool, but that could also work for this.

So for example you could have a custom "runnable" tag not present on subtests, and the presented Go scenario could have a "runnable-darwin" tag to indicate a test only runnable on a certain platform/under certain configurations.

interface TestRunProfile {
  tag?: TestTag;
  // ...
}

interface TestItem {
  tags?: readonly TestTag[];
  // ...
}

class TestTag {
  readonly id: string;
  // maybe more things like a label later, if this is ever exposed in UI
}

[firelizzard18]

@connor4312 That would work for me.

[connor4312]

As we near the end of the iteration, the the test APIs we intend to finalize have been moved to this section (ctrl+f for 122208 if this moves from additional changes in the file): https://github.com/microsoft/vscode/blob/main/src/vs/vscode.proposed.d.ts#L1972. Notably, invalidation is not being finalized. We implemented invalidation and autorun since the Test Explorer UI did so, but autorun is something that most test runners can handle themselves very efficiently, so I would like to remove the notion invalidation and create some mechanism to give extensions control over auto-run instead.

Additionally there were a few changes today, mostly renames:

• vscode.test.* is now vscode.tests.* (like vscode.notebooks.*, which had a similar rename a while ago)
• TestItemCollection.set is now TestItemCollection.replace (set may lead to compile errors as reserved keyword)
• createTestItem is back on the controller
• TestProfileRunGroup is now TestProfileRunKind (although the UI does grouping, the intent expressed in the API is of a type or kind of profile), and the property on the profile was renamed from group to kind
• TestRunState.Unset has been removed, since it isn't used in our current APIs. Additionally, TestMessageSeverity has been removed since we've only seen these to report errors. Both these could be brought back if there's need or use case for them.

Example of a migration: microsoft/vscode-test-adapter-converter@6805807

[firelizzard18]

@connor4312 For Go, I append a message for all test output that can be linked to a specific file. That means, output of the form file.go:13: foo bar gets appended. This is not always error messages. I only ever used a severity of info or error, but a way to mark an appended message as an error or not an error is valuable.

[jdneo]

@connor4312, is there anyway to quickly get the size of the TestItemCollection?

Background: when a test item needs to be removed, and its parent has no other children, I want to remove its parent as well.

[jrieken]

This is not always error messages. I only ever used a severity of info or error, but a way to mark an appended message as an error or not an error is valuable.

The question we're having is about the relation of TestMessageSeverity and TestResultState, esp what does it mean when the result state is Passed but the message is Error and vice versa or different combinations like queued and warning etc

[connor4312]

We discussed this in our call this morning. We're looking at changing the TestRunResult to remove setState and the TestRunState enum entirely, and instead having a method-based API like this:

export interface TestRun {
	enqueued(test: TestItem): void;
	started(test: TestItem): void;
	skipped(test: TestItem): void;
	failed(test: TestItem, message: TestMessage | TestMessage[], duration?: number): void;
	passed(test: TestItem, duration?: number): void;

This is similar to notebook cell execution. In this scenario, TestMessages are tied only to a test failure, and are given only to tests that fail. We propose for next month adding an optional test to associate appendOutput with, for example:

export interface TestRun {
	appendOutput(output: string, test?: TestItem): void;

This allows diagnostic log output to associated with a test, regardless of its passed or failed state. This also gets rid of the weird possible state of having "error" TestMessages for tests that pass.

Please let me know what you think of this proposal. I plan to implement it this afternoon pending feedback, and this plus one more small rename is the only outstanding change we have for finalization at this time.

[connectdotz]

@connor4312 if I understand your proposal correctly, wondering If the purpose of splitting setState() to multiple functions is mainly to take conditional arguments by state, another alternative to consider is a typed-state, such as:

type RunState = 
| {state: 'running'; output?: string}
| {state: 'skipped'; output?:string}
| {state: 'failed'; message: TestMessage|TestMessage[]; duration?:number; output?: string}
| {state: 'passed'; duration?: number; output?: string}
...
interface TestRun {
  setState(item: TestItem, state: RunState);
  ...

typescript will enforce the proper arguments while the API can consolidate multiple API into 1, IMHO, more intuitive setState()...

[firelizzard18]

@connor4312 I want to attach output to a specific location, as in the location property of TestMessage - maybe appendOutput(output: string, location?: Location, test?: TestItem): void?

For more context: When I am running tests to debug a function, I often use Go's test logging facilities to give me more information. This isn't a great example (and I've removed most of the cases for clarity), but here's a screenshot of how it works with TestMessageSeverity and appendMessage:

If I introduce an error, with the previous API this is what I see:

I want to retain this. That is, I want to be able to log non-error output that is attached to a specific location (and test), as well as error output that is attached to a location.

I like the idea of message severity, and I can imagine reasons for wanting to log warning messages that do not cause the test to fail. But even if you kept that, I'm unlikely to support anything other than Info and Error, due to limitations of Go.

Personally, I prefer TestRun.setState over specific methods, but that's just a stylistic thing.

Side note: the UX isn't great when a bunch of test messages are stacked on top of each other:

I would prefer to see all the messages for a given location during a given test run grouped together. I can do this myself in the extension, but I wonder how much utility there is in listing each message individually.

[connor4312]

Thanks for the feedback. Yea, the notion of stacked test messages in the peek is not particularly attractive or intuitive. That was another potential distinction we discussed when considering messages that indicate failure versus general log messages. I'll create an issue for message discussion separately from this issue. The decoration you showed in your first screenshot is an attractive experience that I'm interested in preserving to some degree, but I don't know what that will look like yet.

---

The aforementioned changes have now been made:

• setState is split to specific methods for each state
• TestController.resolveChildrenHandler is now just TestController.resolveHandler

Also, the TestItemCollection now has a size as requested

Example migration: microsoft/vscode-extension-samples@1748ee9

[firelizzard18]

@connor4312 Is this something that can be tested in stable VSCode by setting --enabled-proposed-api? Or is this insiders-only? I ask because @hyangah, part of the team responsible for the official Go extension, was wondering if it would be possible to test with the nightly build of the Go extension golang/vscode-go#1579 (comment).

[connor4312]

We only ship stable ~once per month, so the latest set of API changes will not be in stable, you can only test the old API. The next stable release where these changes are included should arrive when this comment is two weeks old.

[firelizzard18]

@connor4312 Is there any replacement for TestResultState.Errored? I used that if tests failed to compile.

[connor4312]

Would setting TestItem.error be appropriate for that use case?

[JustinGrote]

@connor4312 got a chance to implement today, conversion wasn't too bad. I did have rewrite my "tree walker" function to find all the tests included when a "header" parent testitem is run, I really feel this is something that should be part of the API of TestItemCollection, either as a recurse boolean switch to forEach, or a separate method (walk or something), and could also be applied to the new setstate breakout methods to waterfall down if state is set on a parent testitem.

[firelizzard18]

@connor4312 That's better than nothing, but I preferred the previous UX. It showed (!), which differentiated load errors from test failures: (x). And displaying an error message in the tree view isn't necessarily useful. 1) Only the first line is shown, 2) Go build errors may be multiple lines, and 3) Go build errors are likely already displayed as diagnostics.

[JustinGrote]

For anyone else who struggled with the recursive stuff, here's my helper function

/** Runs the specified function on this item and all its children, if present */
async function forAll(parent: TestItem, fn: (child: TestItem) => void) {
    fn(parent)
    parent.children.forEach((child) => {
        forAll(child, fn)
    })
}

//Example use to set all test results to running
forAll(testParentItem, run.success)

[jdneo]

echo to @firelizzard18. Differentiate errored and failed when setting the test state would be helpful.

The test can be failed due to:

1. The actual result is different from the expected result -- Failed
2. Or, the developer is not setup the tests correctly (For example, the JUnit 4 requires the BeforeAll method must be static, but developer might forget it) -- Errored

BTW, since the TestResultState has Failed and Errored, but when set the state, we can only set Failed looks a little bit strange.

[connor4312]

Okay, I'll bring errored back (as another method on the TestRun). Thanks for the feedback!

[JustinGrote]

@connor4312 the thing I'm most struggling with right now is correlating the tests in the controller with the tests that come back from my adapter. I've been using the id string as a global unique identifier, but now that is only unique to a given testItemCollection, so I have to walk the testController tree to find the matches.

Here are the util functions I made to handle this, they would be better if testItemCollection had an every style method in addition to forEach. A version of these on the testItemCollection for recursive walking of the tree would also be helpful as part of the API.
https://github.com/pester/vscode-adapter/blob/initialCommit/src/testItemUtils.ts