-
Notifications
You must be signed in to change notification settings - Fork 2.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
Implement Test API #12935
Implement Test API #12935
Conversation
2023-09-24.06-23-23.mp4 |
@msujew Can do, but not before next week. |
Sure, I'll take what I get :D I'll probably already review your PR this week, since I'll be on vacation until EclipseCon starting Friday. |
3e396ad
to
31c3806
Compare
@msujew could I get another review-pass? Thursday is release day ;-) |
bcfbe18
to
0f3bba1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, code and functionality look pretty good to me. There are a few things we can probably improve on in the UX department, but this is good to go for a first release.
- Currently, all command buttons are shown for every test item which leads to a lot of UI clutter. VSCode only shows the button on the currently selected tree item.
- VSCode shows the test duration of all tests (similar to the duration of the execution of notebook cells)
- During the discovery of tests, VSCode shows a loading icon on related tree elements. This is currently missing.
- VSCode has dedicated testing theme colors. We are currently using colors from other areas of vscode - which works good enough for now.
I have also left a few minor comments on the code that were only visible on manual inspection of the app.
export const RUN_TEST_WITH_PROFILE: Command = Command.toDefaultLocalizedCommand({ | ||
id: TestCommandId.RunUsingProfileAction, | ||
category: 'Test', | ||
label: 'Execute using Profile... ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Too much whitespace, translation fails due to that.
label: 'Execute using Profile... ' | |
label: 'Execute using Profile...' |
@@ -1,6 +1,6 @@ | |||
{ | |||
"extends": "./configs/base.tsconfig.json", | |||
"include": [], | |||
"include": [ ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
"include": [ ], | |
"include": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Theia workspace preferences have "format on save" enabled and this is what the formatter gives when running Theia blueprint. If that is not correct we should either fix the preference of fix whatever bug is causing that in Theia. Part of the "self-hosting" epic?
if (TestItem.is(t)) { | ||
this.fileSystem.resolve(t.uri!).then(stat => { | ||
if (stat.isFile) { | ||
this.navigationService.reveal(NavigationLocation.create(t.uri!, t.range ? t.range.start : { line: 0, character: 0 })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This range isn't calculated correctly. It seems like the test extensions return range information in the monaco format, where files start at line 1, column 1. But we assume that files start at line 0, column 0, leading to an incorrect offset when executing the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To return ranges in Monaco format would be a clear mistake on the part of the extensions: https://code.visualstudio.com/api/references/vscode-api#Position clearly states 0-based indices. There might be a mistake in the subsequent computations, though 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be, but the behavior is clearly different between vscode and Theia - especially since Theia doesn't place the cursor at the start of the line but after the first character. It might be that we accidentally convert them to the monaco format.
We were missing a bit of css. FIxed.
This PR is by no means meant to be full copy of the VS Code testing UI. Minimal, but functional is the goal. |
@tsmaeder All good, these were just remarks for future work, the PR itself is ready to be merged in my view 👍 (just so people interested in improving stuff have a starting point) |
contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Add Testing panel Start the initial tree structure. (same structure as Test API) No label is shown, no active refresh. Basic css implementation
…t API Project lead: Kevin Clapperton Developers: Zakaria Diabi, Mathieu Bussières Testers: Kevin Clapperton, Théo St-Denis Signed-off-by: Zakaria Diabi <d.zaki2396@gmail.com> Signed-off-by: Mathieu Bussières <mathblin@hotmail.com> Signed-off-by: Théo St-Denis <theo.st-denis@polymtl.ca> Signed-off-by: Kevin Clapperton <keclapperton@gmail.com> Co-authored-by: Mathieu Bussières <mathblin@hotmail.com> Co-authored-by: Théo St-Denis <theo.st-denis@polymtl.ca> Co-authored-by: Kevin Clapperton <keclapperton@gmail.com>
- Adapted plugin-side services to work with Theia test service - Added views for test runs, results and output - Built out Test Explorer view - Added various commands to run tests and manage results The result is a minimal, but functional UI for working with tests Fixes eclipse-theia#10669 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
c8fb1d5
to
008f53f
Compare
What it does
Closes #10669
Implements support for the VS Code testing API in the usual fashion: there is a test service interface and implementation that is plugin-agnostic (
packages/test?
) and a plugin-specific implementation in the plugin-ext package.How to test
There is a sample test controller contribution active in the sample applications via the "api-samples" folder. The test controller shows up in the Test Explorer View. You'll need to invoke the "Add Some Tests" command to populate the controller with tests.
In addition, I used the https://open-vsx.org/extension/ms-playwright/playwright extension to test the code on the Theia source code itself. On Windows, you'll need to fix the playwright "theia:start" yarn task like so:
Note that this is minimal implementation that lets us satisfy the test API. It consists of "Test Exlorer" and "Test Result" views showing up in the left sidebar and a "Test Output" and a "Test Result" view showing up at the bottom. You'll have to open those second two views manually.
Follow-ups
While the API is complete, there are a couple of things which the UI will never use:
Even if a test plugin supports continoous runs, the UI will never invoke a test run in this fashion.
invalidateTestResults
.The UI has no support for showing test results as outdated, so that API has no function
We show no marker annotations for tests in the editor. To run tests, you'll need to use the UI in the Test Explorer view.
Since the whole coverage API is in "proposed" state, it is not implemented.
Review checklist
Reminder for reviewers