Support TestMessageStackFrame API #14154

rschnekenbu · 2024-09-11T14:38:51Z

What it does

Adds 'stacktraces: TestMessageStackFrame' to TestMessage. This optional property allows extensions to describe the context in which an error could occur when test is failing.
Here is a snapshot as an example on vscode. They embed small monaco editors to display the line and a bit around to display the stack trace when the test is failing.

Note that our current implementation of Test is lighter than the vs code one. So a more simple approach was given, with only the label of the StackTrace and a hyperlink to navigate to the file located at the uri given by the stack frame, as well as the cursor position.

Fixes #14111

Contributed on behalf of STMicroelectronics

How to test

The attached extension is based on test-sample-provider from vscode-extension-samples:

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

The testing extension will create tests for each mathematical expressions located in markdown files. It will also randomly add 0 to 3 stack traces to the TestMessage when the test is in failure, to test the cases with 0, 1 or more stack frames in test message.
Have several md files in the workspace, with some computation in it. For my testing, I used the following set of files:

test.zip

Runs some of the tests from test explorer, with some ones failing. Ensure that the messages are displayed in the Test Results view and that the links can be navigated to the files in the workspace. The cursor shall be placed in the right location.

Follow-ups

None known

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

Also properly convert TestState back from TestStateChangeDTO. fixes eclipse-theia#14111 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>

tsmaeder · 2024-09-12T08:21:19Z

packages/plugin-ext/src/common/test-types.ts

+export namespace TestFailureDTO {
+    export function is(ref: unknown): ref is TestFailureDTO {
+        return isObject<TestFailureDTO>(ref)
+            && ref.state === (TestExecutionState.Failed || TestExecutionState.Errored);


This does not work: (TestExecutionState.Failed || TestExecutionState.Errored) will always evaluate to TestExecutionState.Failed.

Indeed, thanks for catching. Fixed in 2nd commit

tsmaeder · 2024-09-12T08:24:45Z

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

-                this.testStates.set(item, change);
-                stateEvents.push({ test: item, oldState: oldState, newState: change });
+                // convert back changes (for example, convert back location from DTO)
+                const convertedState = convertTestState(change);


Why do we need this conversion now? Aren't the types compatible anymore?

No, the Position for example are not the same structure (line vs lineNumber for example). There is also the issue of 0-based or 1-based indexes on Position and Range.
TestMessage location is not used for example, but it still needs conversion to be exact for usage in the future. Same for properties of TestMessageStackFrame.

Yes, but Position is an interface that we define ourselves in this PR, so we can make it compatible. As for the zero/one based index conversion, aren't we doing the on the ext side in general? If we have an explicit TestStateChangeDTO we can design it in a way that is compatible with the browser side structure, no?

That can be done, while adding some new converters on ext side to the language server types.
I pushed a new version that removes this conversion.

tsmaeder · 2024-09-12T11:32:08Z

packages/test/src/browser/view/test-result-widget.ts

+            frameElement.append(' from ');
+            const link = this.node.ownerDocument.createElement('a');
+            link.textContent = `${this.labelProvider.getName(uri)}`;
+            link.title = `${uri}`;


Using title here instead of href prevents the UI from rendering the anchor tag as a proper link. No underlining and the cursor does not change to a pointing hand when hovering. Any particular reason for doing it this way?

No, no particular reason. That's fixed.

tsmaeder · 2024-09-12T11:35:43Z

packages/test/src/browser/view/test-result-widget.ts

+        if (stackFrame.uri) {
+
+            const uri = stackFrame.uri;
+            frameElement.append(' from ');


I18N, also, it would be nice if we could make this look a bit like a stack trace, i.e. similar to

Uncaught (in promise) TypeError: Cannot destructure property 'startLineNumber' of 'range' as it is undefined. at MonacoOutlineContribution.asRange (monaco-outline-contribution.ts:260:17) at MonacoOutlineContribution.getFullRange (monaco-outline-contribution.ts:281:21) at MonacoOutlineContribution.parentContains (monaco-outline-contribution.ts:245:62) at MonacoOutlineContribution.applySelection (monaco-outline-contribution.ts:227:26) at MonacoOutlineContribution.createRoots (monaco-outline-contribution.ts:156:14) at MonacoOutlineContribution.updateOutline (monaco-outline-contribution.ts:113:43) at monaco-outline-contribution.ts:97:92 at event.ts:176:42 at CallbackList.invoke (event.ts:184:26) at Emitter.fire (event.ts:327:36)

The message looks like more like a stack trace now.

rschnekenbu · 2024-09-16T15:48:13Z

I will squash and update the changelog once the PR is accepted.
As a side node, I also updated the test css file so the Test Runs view now uses the .theia-test-run-view class rather than the theia-test-results-view. That sounds more coherent, and theia-test-results-view can now be used in the Test Results view.

- remove conversion on test package - adapt DTOs to allow 1-to-1 mapping on browser side - add required convertes on plugin-ext side

tsmaeder · 2024-09-19T11:29:41Z

@rschnekenbu there now seems to be a conversion problem: when I click on the line at frame 0 (test5.md:5:0), the file test5.md is opened, but the cursor is placed on line 6. Sounds like an missing +/- 1 somewhere.

rschnekenbu · 2024-09-19T15:20:23Z

Indeed, the location which is used to navigate is based on the 0-based one, but the tool obviously displays the Monaco coordinates. So I will fix the location shown in the Test Results to be the Monaco location.

Support TestMessageStackFrame API

8bfe22d

Also properly convert TestState back from TestStateChangeDTO. fixes eclipse-theia#14111 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>

rschnekenbu force-pushed the issues/14111 branch from afa8bc0 to 8bfe22d Compare September 11, 2024 14:40

rschnekenbu requested a review from tsmaeder September 12, 2024 08:01

tsmaeder reviewed Sep 12, 2024

View reviewed changes

address review comments

298f046

Merge branch 'master' into issues/14111

31723a7

rschnekenbu requested a review from tsmaeder September 17, 2024 09:47

address review comments

5ccfa86

- remove conversion on test package - adapt DTOs to allow 1-to-1 mapping on browser side - add required convertes on plugin-ext side

rschnekenbu and others added 2 commits September 23, 2024 09:24

Merge branch 'master' into issues/14111

a3af47d

Adapt position shown to the user for the link to 1-based monaco position

fd2dd2e

tsmaeder approved these changes Sep 23, 2024

View reviewed changes

rschnekenbu merged commit 88b64b2 into eclipse-theia:master Sep 23, 2024
11 checks passed

rschnekenbu deleted the issues/14111 branch September 23, 2024 13:20

sgraband added this to the 1.54.0 milestone Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TestMessageStackFrame API #14154

Support TestMessageStackFrame API #14154

rschnekenbu commented Sep 11, 2024 •

edited

Loading

tsmaeder Sep 12, 2024

rschnekenbu Sep 16, 2024

tsmaeder Sep 12, 2024

rschnekenbu Sep 16, 2024

tsmaeder Sep 17, 2024

rschnekenbu Sep 17, 2024

tsmaeder Sep 12, 2024

rschnekenbu Sep 16, 2024

tsmaeder Sep 12, 2024

rschnekenbu Sep 16, 2024

rschnekenbu commented Sep 16, 2024

tsmaeder commented Sep 19, 2024

rschnekenbu commented Sep 19, 2024

Support TestMessageStackFrame API #14154

Support TestMessageStackFrame API #14154

Conversation

rschnekenbu commented Sep 11, 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

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

rschnekenbu commented Sep 16, 2024

tsmaeder commented Sep 19, 2024

rschnekenbu commented Sep 19, 2024

rschnekenbu commented Sep 11, 2024 •

edited

Loading