[vscode] Support TestRunRequest.preserveFocus #13839

rschnekenbu · 2024-06-24T09:10:53Z

What it does

Add a progress service for tests to support activation of views
Support vscode API to listen for test run request preserveFocus:

when preserve focus is true, nothing had to be done
if preserveFocus is false, then the Test Results view shall be activated.

fixes #13759

contributed on behalf of STMicroelectronics

How to test

Start the theia browser example on master and install the following extension (derived from vscode sample extension test-provider-sample):

src: test-provider-sample-0.0.1-src.zip
zip vsix: test-provider-sample-0.0.1.zip

When starting extensions from Test UI, nothing should happen (as preserveFocus is always true in this case)
With the PR and the extensions, 2 additional commands are available with the markdown tests: Test Sample: Trigger Tests (preserveFocus: true/false). With the one with true, nothing shall happen also. The Test Result view shall activate when using the command with the false preserve Focus.

Follow-ups

The current implementation may be extended later based on preferences and test results status. In VS Code, when tests are started or failing, the Test Result view or the Test Explorer view can be activated based on user preferences (see https://github.com/microsoft/vscode/blob/f9238dd94a8b54dc3adb274927c7871e7c215300/src/vs/workbench/contrib/testing/browser/testingProgressUiService.ts#L49)

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

rschnekenbu · 2024-06-24T12:57:50Z

packages/plugin-ext/src/common/plugin-api-rpc.ts

Not really sure about naming here. Maybe $notifyTestRunRequested may be more aligned with the other methods?

+1 for testRunRequested consistency is king.

tsmaeder

First reading of the code.

tsmaeder · 2024-06-25T07:33:57Z

packages/plugin-ext/src/common/plugin-api-rpc.ts

+1 for testRunRequested consistency is king.

packages/plugin/src/theia.d.ts

tsmaeder · 2024-06-25T08:06:45Z

packages/test/src/browser/view/test-execution-progress-service.ts

+// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
+// *****************************************************************************
+
+import { inject, injectable } from '@theia/core/shared/inversify';


Not sure why we need this class. Why isn't this handled in TestService?

Or alternatively, don't go through the test TestService but notify this service directly from test-main.ts.

I had an issue with the architecture there: the test views depend on the TestService, so I could not have the TestService to depend on them.
But yes, I can notify it from test-main.ts. I will propose a new version with TestExecutionProgressService directly used in TestingMain.

The new commit has shifted a bit the way it is architectured, as you suggested. TestingMain is directly invoking the progress service and not channeling events through main test service anymore.

packages/plugin-ext/src/plugin/tests.ts

tsmaeder · 2024-06-25T08:18:01Z

packages/plugin-ext/src/main/browser/test-main.ts

@@ -624,4 +625,8 @@ export class TestingMainImpl implements TestingMain {
    $notifyTestRunEnded(controllerId: string, runId: string): void {
        this.withController(controllerId).withRun(runId).ended();
    }
+
+    $notifyOnTestRunRequest(preserveFocus: boolean): void {


Can't we use $notifyTestRunCreated instead of adding a new notification? A test run starts either at creation or when it's "running" flage changes, right?

done on new commit.

tsmaeder · 2024-06-25T16:07:46Z

packages/test/src/browser/test-execution-progress-service.ts

+@injectable()
+export class DefaultTestExecutionProgressService implements TestExecutionProgressService {
+
+    @inject(WidgetManager)


This seems unused.

tsmaeder · 2024-06-25T16:08:06Z

packages/test/src/browser/test-execution-progress-service.ts

+    protected readonly widgetManager: WidgetManager;
+
+    @inject(ApplicationShell)
+    protected readonly shell: ApplicationShell;


tsmaeder · 2024-06-25T16:08:15Z

packages/test/src/browser/test-execution-progress-service.ts

+    @inject(TestViewContribution)
+    protected readonly testView: TestViewContribution;
+
+    @inject(TestService)


seems unused.

tsmaeder · 2024-06-25T16:20:00Z

@rschnekenbu I have a couple of remarks from testing:

The preferences are in the "Extensions/Testing" section, but IMO should be in "Features/Testing".
When I close the test explorer view, it is not opened when I run the tests with "preserveFocus: false".
Not sure what the distinction is between "openOnStart" and "openExplorerOnStart" 🤷

rschnekenbu · 2024-06-26T08:14:38Z

I moved the preferences to right place. That is now under Features section.
I removed the openWithTestExplorer, as the option (copied from vs code) is not really obvious given the comment on the d.ts file and in the description of the preference. Now, the 2 options are 'do nothing' or 'open Test Results View'. I kept however the enum choice if we want to expand later the choices to other views.
I also removed unused code left from previous implementation.

rschnekenbu · 2024-06-26T08:16:31Z

A small detail about the test of this feature: Running the tests from the UI should preserve the focus in any case, and not open any views. Only triggering from the extension, with the 'preserveFocus:false' version should open the view if the preference is set for this.

tsmaeder · 2024-06-27T08:29:41Z

@rschnekenbu for me revealing the test explorer does not work in this scenario:

Set the preference to openOnTestStart.
Make sure the file explorer view is visible
Invoke `Trigger Test (preserveFocus: false)
Observe: the test explorer view is not revealed.

🤷

rschnekenbu · 2024-06-27T08:31:14Z

@rschnekenbu for me revealing the test explorer does not work in this scenario:

Set the preference to openOnTestStart.

Make sure the file explorer view is visible

Invoke `Trigger Test (preserveFocus: false)

Observe: the test explorer view is not revealed.

🤷

The Test Results View shall open, not the Test Explorer. Is that not the case?

tsmaeder · 2024-06-27T08:35:26Z

The Test Results View shall open, not the Test Explorer. Is that not the case?

No, the file explorer still is visible.

tsmaeder · 2024-06-27T08:40:37Z

Cleared this up: The "Test Result View" is the one that shows up in the lower right corner where the terminals, etc. are.

Add a progress service for tests to support activation of views Forward Test Run to be started event to progress service to activate view if needed fixes eclipse-theia#13759 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>

rschnekenbu force-pushed the issues/13759 branch from f581b5a to 811c04d Compare June 24, 2024 12:09

rschnekenbu marked this pull request as ready for review June 24, 2024 12:55

rschnekenbu requested a review from tsmaeder June 24, 2024 12:55

rschnekenbu commented Jun 24, 2024

View reviewed changes

tsmaeder reviewed Jun 25, 2024

View reviewed changes

rschnekenbu requested a review from tsmaeder June 25, 2024 14:21

tsmaeder reviewed Jun 25, 2024

View reviewed changes

rschnekenbu requested a review from tsmaeder June 26, 2024 09:12

rschnekenbu mentioned this pull request Jun 26, 2024

Bump API version to 1.90.2 #13849

Merged

2 tasks

rschnekenbu added the vscode issues related to VSCode compatibility label Jun 26, 2024

tsmaeder approved these changes Jun 27, 2024

View reviewed changes

rschnekenbu force-pushed the issues/13759 branch from 7ff7879 to 33ea608 Compare June 27, 2024 08:45

rschnekenbu merged commit 0e50cfc into eclipse-theia:master Jun 27, 2024
13 of 14 checks passed

rschnekenbu deleted the issues/13759 branch June 27, 2024 08:52

jfaltermeier added this to the 1.51.0 milestone Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vscode] Support TestRunRequest.preserveFocus #13839

[vscode] Support TestRunRequest.preserveFocus #13839

rschnekenbu commented Jun 24, 2024 •

edited

Loading

rschnekenbu Jun 24, 2024

tsmaeder Jun 25, 2024

tsmaeder left a comment

tsmaeder Jun 25, 2024

tsmaeder Jun 25, 2024

tsmaeder Jun 25, 2024

rschnekenbu Jun 25, 2024

rschnekenbu Jun 25, 2024

tsmaeder Jun 25, 2024

rschnekenbu Jun 25, 2024

tsmaeder Jun 25, 2024

tsmaeder Jun 25, 2024

tsmaeder Jun 25, 2024

tsmaeder commented Jun 25, 2024

rschnekenbu commented Jun 26, 2024 •

edited

Loading

rschnekenbu commented Jun 26, 2024

tsmaeder commented Jun 27, 2024

rschnekenbu commented Jun 27, 2024

tsmaeder commented Jun 27, 2024

tsmaeder commented Jun 27, 2024

[vscode] Support TestRunRequest.preserveFocus #13839

[vscode] Support TestRunRequest.preserveFocus #13839

Conversation

rschnekenbu commented Jun 24, 2024 • edited Loading

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Jun 25, 2024

rschnekenbu commented Jun 26, 2024 • edited Loading

rschnekenbu commented Jun 26, 2024

tsmaeder commented Jun 27, 2024

rschnekenbu commented Jun 27, 2024

tsmaeder commented Jun 27, 2024

tsmaeder commented Jun 27, 2024

rschnekenbu commented Jun 24, 2024 •

edited

Loading

rschnekenbu commented Jun 26, 2024 •

edited

Loading