-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
API for async test items #126987
Comments
@jrieken One benefit of async loading test items is that in the test explorer, the tests can be loaded level by level(when user expands the node deeper and deeper). Especially in a large project which might contains thousands of test items, the extension doesn't have to load all the tests at one time. So one (maybe silly) question: If we make the test items synchronized, will the extension get a signal when a test node is expanded? (like currently, |
Language: Happens that a user complains about the slow loading of the tests. Async would solve this. What I think is best for my case:
|
Understood - so this is mostly about breath first instead of depth first. It makes sense to me but it also means we need to polish the The catch with
That means the "resolving" applies to a much longer span than the execution of the handler and likewise the cancellation token represents the period for which VS Code has "special" interest in an item. The alternative proposal would do away with the resolve-callback and add an event which fires whenever a test item is interesting (expanded in the tree, opened as text document) and vice versa. Something like interface TestController {
readonly onDidChangeInterest: Event<{ item: TestItem<T>, interesting: boolean }> // needs better names
} VS Code would "blindly" fire these events and extensions can use this as signal to prioritize resolving of items, and/or start watching for changes etc. Similar to how extensions are listening to other events, like |
Joh and I talked a bit about this this morning. What we're thinking of is making three changes:
This solves a few problems:
|
See #126987 (comment) This commit makes the following changes: - Keep the `resolveChildrenHandler`, and remove the CancellationToken parameter - Remove `TestITem.status` and instead have `TestItem.canResolveChildren?: boolean` - Add `TestItem.busy?: boolean`. Note that the UI would implicitly show the item as busy while `resolve` is being called. (This is a new feature) Upgrading to account for these changes should take around 10 to 20 minutes: 1. Where you previously set `item.status = vscode.TestItemStatus.Pending`, instead set `item.canResolveChildren = true`. 2. If you used the cancellation token in resolveChildrenHandler, you no longer need to do so. What you do here instead is up to you: - If you set up global workspace watchers, add those to `context.subscriptions` - You _probably_ don't need to set up watchers for "file" test items, since you will receive updates via `vscode.workspace.onDidChangeTextDocument` and/or any FileWatcher you have set up. Example of an update: microsoft/vscode-selfhost-test-provider@7287c64
I've made these changes in df5cd47. However I went with |
@hbenl, @jdneo, @matepek, @OmarTawfik Sorry for another ping but I wonder "how deeply asynchronous" your tests are. I understand that discovery and processing of test files is two steps but I wonder how many async steps follow when a test file is processed:
I wonder, iff step 2 is ever async/lazy or always a single sync pass. E.g. we delay loading a Java class file but once it is loaded discovery of tests is a single pass and sync, right? The same for AST- or execution-based discovery, right? Iff so we might be able to simplify the API further, e.g have managed test containers and short lived test items. The advantage would be that extensions only need to create and return test items from their source of truth but never dispose nor update them (VS Code would handle all of that). The disadvantage is that some flexibility/complexity goes, esp. the case of "i have lazily discovered this suite and I now delay discovery of its tests because ???" Now I wonder if multi-level lazy test items are real or just a theoretical problem? There is some API sketch for how it could look here: #127143 (comment) |
Some input from Java: Test HierarchyIn Java, the test hierarchy looks like this:
Test Discovery -- All async calls
About the parent pointerCurrently, the parent pointer can be useful in one scenario: In Java, we allow other extension request a customized test running and send that request to the test runner extension. The workflow looks like this: Other extension register their commands to the test explorer via The problem here is, since the test node passed to the test runner is a different instance than the real one that is created by the test runner itself. So currently we use the parent pointer to trace back from the target node to root in the test tree, and then use that path to find the real node. This is because we use a WeakMap to store the data for each node, so the instance must be the same to get the data information. Though we can use Map and the id as the key to get rid of that, the advantage of using WeakMap is that we don't need to take care about deleting entries as long as VS Code will dispose the test item. |
@jdneo What is corresponding output in terms of Do you have a code pointer for me so that I can understand how the current API is being used? |
Yes, all the node belongs to the test hierarchy will be a test item. The layout looks like this: Meanwhile, you can find the build here if you want to have a try: microsoft/vscode-java-test#1162. The implementation can be found here: https://github.com/jdneo/vscode-java-test/blob/cs/new-test-api/src/controller/testController.ts |
Very helpful. Thanks @jdneo. |
This is part of #122208 and a continuation of #115089.
Today, there is two APIs for asynchronously resolving test items:
TestItem.status: TestItemStatus
-API which is what @hbenl has suggested in Allow tests to be loaded asynchronously #115089 (comment). It allows extension to signal that an item isn't fully loaded yet.TestController.resolveChildrenHandler
which tells extensions that an item should become "Resolved" and when an item isn't interesting anymore (via the cancellation token)There is the following issues with this approach
resolveChildrenHandler
is a first for the API and each "first" is being questioned by defaultI also want to take a steps back and revalidate that we still need async test items. Many changes have happened, the
TestController
now sports a push-model and that allows extensions to add and remove test items any time and by their own schedule. The overall set of test items is fluid, there is no more thing likeonDidDiscoverInitialTests
, and extensions are in full control.So, the only use-case of async test items is when extensions "know that a file/thing contains tests but hasn't looked into it". The only value of that is the rendering of an empty tree node in the testing explorer.
⬆️ @hbenl, @jdneo, @matepek, @OmarTawfik How often is this a problem? Assuming you can add/remove resolved test anytime and assuming you can lets us know that you are currently computing tests would still need to support for unresolved, shallow test items that need to be filled in later?
The text was updated successfully, but these errors were encountered: