Skip to content
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

vscode.TestObserver is an unusual API pattern #117379

Closed
jrieken opened this issue Feb 23, 2021 · 5 comments
Closed

vscode.TestObserver is an unusual API pattern #117379

jrieken opened this issue Feb 23, 2021 · 5 comments
Assignees
Labels
api testing Built-in testing support under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Feb 23, 2021

Testing #117307

So far, my understanding of vscode.TestObserver and its factory functions is that a test observer gives you access to tests, e.g it enumerates them and fires events when something changes. The usual API pattern for something like that doesn't use such an indirection but exposes things directly. So, I wonder why there isn't something like this instead

namespace test {
   const testItems: TestItem[];
   const onDidChangeTestItems: Event<TestItemChangeEvent>
}
@connor4312
Copy link
Member

connor4312 commented Feb 23, 2021

The reasoning is that the test API is built to be lazy. We don't want to load tests, particularly tests for the workspace, unless there's someone listening to it -- in some cases (like Java) watching tests can be pretty expensive.

There is also an optimization for the common case of documents: consumers can request test results for a single document, which can be used to provide decorations, code lenses, etc. This can often be much cheaper than watching the entire workspace for tests and changes.

Both of these things are hard to express just given a TestItem array and change event.

Also fyi, the test observer api is much rougher than the rest of the API and I do not plan to stabilize it under after the provider-related API is available

@connor4312 connor4312 added testing Built-in testing support under-discussion Issue is under discussion for relevance, priority, approach labels Feb 23, 2021
@jrieken
Copy link
Member Author

jrieken commented Feb 24, 2021

We don't want to load tests, particularly tests for the workspace, unless there's someone listening to it

Is that equal to opening the test view or to running some "run all tests" command?

@connor4312
Copy link
Member

Both of those requires all tests to be enumerated, yea. Whereas running tests in a document -- through the various "current file" commands or by clicking a gutter decoration -- only requires tests for the document.

@connor4312 connor4312 added the api label Mar 4, 2021
@connor4312 connor4312 added this to the Backlog milestone Apr 16, 2021
@connor4312 connor4312 modified the milestones: Backlog, May 2021 May 12, 2021
@connor4312
Copy link
Member

I think there is a better understanding of the load demands around tests now 🙂 However I did simplify things in 4f4b3ca

  • The TestObserver is cleaner now:

    /**
    * A TestObserver is returned from `vscode.test.createWorkspaceTestObserver`
    * or `vscode.test.createDocumentTestObserver`. It can be used to introspect
    * tests published by other extensions.
    */
    export interface TestObserver {
    /**
    * List of tests returned by test provider for files in the workspace.
    */
    readonly tests: ReadonlyArray<ObservedTestItem>;
    /**
    * An event that fires when an existing test in the collection changes or
    * its list of children change. If a root test was added or removed,
    * then `null` is returned.
    */
    readonly onDidChangeTest: Event<ObservedTestItem | null>;
    /**
    * Dispose of the observer, allowing VS Code to eventually tell test
    * providers that they no longer need to update tests.
    */
    dispose(): void;
    }

  • I share an interface between the TestResultSnapshot which has common TestItem properties. Then it also has a getChildren(): Promise method.

    /**
    * A {@link TestItem}-like interface returned in the {@link TestObserver}.
    */
    export interface ObservedTestItem {
    /**
    * The parent of the test item, or undefined if there is none.
    */
    readonly parent?: ObservedTestItem;
    /**
    * Requests and retrieves the children of the test item. This method will
    * not be present if there is no {@link TestItem.resolverHandler} on the
    * original item.
    */
    getChildren?(): Thenable<ObservedTestItem[]>;
    }

  • I left the test results mostly as is. These are pretty straightforward, static items already. (Test results are only published there once the test completes. This means liveshare cannot show the results as they're running, but that is a single use case and in my opinion not worth exploding API surface for)

    /**
    * List of test results stored by VS Code, sorted in descnding
    * order by their `completedAt` time.
    * @stability experimental
    */
    export const testResults: ReadonlyArray<TestRunResult>;
    /**
    * Event that fires when the {@link testResults} array is updated.
    * @stability experimental
    */
    export const onDidChangeTestResults: Event<void>;

The exposed APIs do not allow the same optimal paths that core leverages around tests, but in exchange it results in a much simpler and more standard API.

@jrieken
Copy link
Member Author

jrieken commented Oct 15, 2021

This has become obsolete

@jrieken jrieken closed this as completed Oct 15, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api testing Built-in testing support under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

2 participants