Test-Provider-Sample: Continuous RunHandler registers redundantly

[JustinGrote]

@connor4312

https://github.com/microsoft/vscode-extension-samples/blob/ead2640188c31d12ad07ed4d3c0d47d5b54a23c8/test-provider-sample/src/extension.ts#L9C1-L23C4

In this section, the runhandler registers an event for when files change to trigger a test run request if continuous is set, which from my testing only occurs currently when someone clicks the "eye" next to a test. The problem is that it is not test-level based but URI based, so it will both re-run all tests in the markdown file and not just the ones specifically specified, and also if you flag multiple tests in the same file for continuous, multiple full-file handlers get registered, so say you flag two different tests in the same file, the test run will happen twice upon saving changes to the file.

If I am understanding this correctly, a better approach would be upon a file change detected, first identify which tests changed, and then have a flag somewhere such as the testItemData where it shows if the test should be continuous or not, and then run it if that is the case. An alternative would be to register an event for each individual test item that either runs the tests in parallel, or calls a debouncing function so they can all be run together after a short debounce delay (depending on whether the test runner is more efficient on a per-test or per-test-file basis).

[JustinGrote]

For my Pester Tests extension, I ended up doing the following:

1. Have a continuousTests Set per controller, that when a run request with continuous comes in, populate it and wire up a removal to the cancellationToken. A test run won't start upon clicking the eye on purpose
2. Whenever test discovery occurs from any source, check if any changed test is present either in continuousTests directly or the children of those items. If it is, initiate a test run (my runHandler is debounced so if a lot of these edits come in at once, they will collate to a single run since Pester works best if as many testItems as possible are included in a single run)

An alternative approach would be to use events and register events on a testItem basis rather than a Uri basis, if your testRunner doesnt need debouncing.

[connor4312]

This is kind of an artifact of the way the sample test provider is set up: a test identity is defined by the equation its testing, so changing a test will actually delete the old test and make a new one with a different equation. Therefore watching at a file level is the rough approximation of what continuous mode is meant to represent.

[JustinGrote]

@connor4312 the issue is more this (to reproduce):

1. Mark a test for continuous run
2. Mark another test for continuous run
3. When the second test is marked for continuous run, there are now two full-file test runs happening because two full-file event listeners are registered.
4. Mark a third and then 3 test runs will happen simultaneously rather than just 1, etc. and all 3 test runs contain redundant tests.

EDIT: As an extreme extrapolation of above, I marked 5 different tests for continous run, and then saved a change to the file once. Notice 5 redundant job runners kicked off (because 5 full-file listeners are now registered)

[connor4312]

It's up to the test extension as to how they want to handle that multiple watch requests. I should update the sample to deal with it better. Watching a test requires the extension maintains state, and it's expected that further watches, if precise per-test watching is not possible (as in the sample extension) that the test extension deduplicates at whatever granularity is appropriate. In this case VS Code does not 'know' that watching two different tests in the same file results in duplicated behavior in the extension side.

[JustinGrote]

@connor4312 agreed, I was just noting in this case there's an issue with how the sample currently handles it.

[JustinGrote]

@connor4312 thanks! 🤘