Sidebar support #357

gstamac · 2018-08-06T09:18:37Z

I implemented Test sidebar support for vscode. It is just a first version where you can see the status of test run and locate the test by clicking on it. It will also show the test result (with errors) in the tooltip. Next version should support some commands like run and debug.
jest-editor-support has a shortcoming and will not parse describe blocks (described here #351). That means it cannot distinct between tests with the same name in the same file. So now when you click on such test it will always put the cursor on the first one. When describe block parsing will be implemented I can update sidebar to support it also.
Here are the screenshots....

# Conflicts: # package.json # src/JestExt.ts # src/Settings/index.ts # src/diagnostics.ts # src/extension.ts # tests/JestExt.test.ts # tests/diagnostics.test.ts # tests/extension.test.ts

gstamac · 2018-08-14T13:53:16Z

Has anybody reviewed this?

connectdotz · 2018-08-17T21:15:51Z

Usually, a PR is ready for review when it passes the ci tests, unless there is a good reason for an exception...

@gstamac looks like the ci test didn't finish travis test suite, maybe some new tests trigger async ops that stopped them from completing? Feel free to discuss here is you need some help.

coveralls · 2018-08-20T10:57:53Z

Pull Request Test Coverage Report for Build 488

117 of 121 (96.69%) changed or added relevant lines in 5 files are covered.
41 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.5%) to 85.499%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/JestExt.ts	3	4	75.0%
src/SideBar/JestTreeNode.ts	58	59	98.31%
src/SideBar/JestTreeProvider.ts	28	29	96.55%
src/extension.ts	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/extension.ts	1	69.44%
src/JestExt.ts	40	64.73%

Totals
Change from base Build 449:	1.5%
Covered Lines:	807
Relevant Lines:	931

💛 - Coveralls

gstamac · 2018-08-20T13:37:10Z

It looks like Travis has problems with console.log. I removed it from a test (not my test) and now it works. I also added changelog entry.

connectdotz

Hey @gstamac thanks for the PR, this is a highly requested feature.

However, I didn't get very far when trying to test run it... it crashed in a vanilla CRA app. I took a quick pass to call out a few obvious issues, please take a look. thx.

connectdotz · 2018-08-21T06:12:30Z

src/SideBar/TestResultTree.ts

+  }
+
+  private parseAssertionResults(results: JestAssertionResults, parsedResults: TestResult[]) {
+    const suite = this.getSuite((<any>results).ancestorTitles, this.suites)


this crashed when the test file doesn't have the "describe" block outside of the "it" blocks, such as a default CRA app:

results.assertionResults=[{"failureMessages":[],"status":"passed","title":"renders without crashing"}]

TypeError: Cannot read property 'forEach' of undefined
at TestResultFile.getSuite (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/TestResultTree.js:28:16)
at TestResultFile.parseAssertionResults (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/TestResultTree.js:11:28)
at TestResultFile.results.assertionResults.forEach.r (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/TestResultTree.js:8:52)
at Array.forEach ()
at new TestResultFile (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/TestResultTree.js:8:34)
at JestTreeProvider.loadTestResultsForFile (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/JestTreeProvider.js:35:16)
at data.testResults.map.r (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/JestTreeProvider.js:30:58)
at Array.map ()
at JestTreeProvider.loadTestResults (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/JestTreeProvider.js:30:44)
at JestTreeProvider.refresh (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/SideBar/JestTreeProvider.js:15:14)
at JestExt.updateWithData (/Users/vsun/ConnectDotz/External/vscode-jest/out/src/JestExt.js:304:30)

connectdotz · 2018-08-21T06:17:47Z

src/JestExt.ts

@@ -364,6 +369,8 @@ export class JestExt {
    const statusList = this.testResultProvider.updateTestResults(normalizedData)
    updateDiagnostics(statusList, this.failDiagnostics)

+    this.sidebarProvider.refresh(data)


not sure if we should process all the tests up front like this, giving we will only need to display the tree when the file is actually displayed in the editor... please see if you can move the logic to triggerUpdateActiveEditor instead.

The tree is displayed every time the tests are run not only when the file is selected. You probably mean the outline tree but what we are showing here is the test sidebar tree.

indeed, I was thinking about the outline view, my bad.

trying it again on vscode-jest itself, added a few fake tests and encountered the following issues:

looks like under the root "Tests" are the describe blocks instead of the file. So if a test file doesn't have a root describe block or it has multiple parallel describe blocks, they will not show any hierarchical relationship. Is that intended? Is there a particular reason we don't want to use the test file for grouping?

(update) ok, I saw you have a jest.showFilesInSidebar setting, however

when I update the setting, the sidebar display didn't change until I restart/reload vscode.

it displays the full path instead of the filename, which is by far the most important info but now is almost always truncated... can we find a way to display the filename first, maybe use tooltip or other visual hints for path if necessary?

on the other hand, I wonder if there is really a common use case without grouping the tests by file... maybe at least to make the default of jest.showFilesInSidebar to true?

the root level it blocks are not shown, and if reload/restart vscode, it crashed the plugin, put it in the never-ending spinning "Starting watch mode" and the Test sidebar is empty... (same error as mentioned in earlier comment.)

I added support for root tests, removed the folder name from filename and changed the default setting to show files by default. My practice has always been to have all tests inside describe and to have only one describe per file. That's why I chose the implementation I did.
I see you have to reload the window when the settings change but I think that's the case for all vscode-jest settings because getExtensionSettings is called on extension.activate event.

I see you have to reload the window when the settings change but I think that's the case for all vscode-jest settings because getExtensionSettings is called on extension.activate event.

Please take a look at triggerUpdateSettings

connectdotz

the crash is gone, 👍

a few usability issues:

if watch-mode only triggered partial tests, the test sidebar would show the partial results instead of the full test results like you intended.
I saw you now display the file nodes with the relative path, which is definitely shorter, but it could still truncate the actual file name. I wonder, instead of a flat file-path structure, how about a hierarchical one?

following are nice-to-have:

can we have a command or shortcut to do the on-demand "deep" collapse or expand, it's like toggling the jest.autoExpandSidebar settings, so developers can quickly zoom in/out dynamically.
in the explorer view, the file in the active editor gained autofocus; is it possible to do the same with the test sidebar? at least scroll/expand to the file level if not the test level by cursor...

# Conflicts: # CHANGELOG.md

gstamac · 2018-09-25T10:29:09Z

Finally had some time to look into this again.

if watch-mode only triggered partial tests, the test sidebar would show the partial results instead of the full test results like you intended.

I fixed the partial test run so it updates the results. The match is done based on file name so if you delete the test file partial run will not notice it. But if you stop/start jest it will rerun all tests and refresh the whole tree.

I saw you now display the file nodes with the relative path, which is definitely shorter, but it could still truncate the actual file name. I wonder, instead of a flat file-path structure, how about a hierarchical one?

I don't agree with this. If you have a deep tree structure this will look terrible. I think this is OK for first version.

can we have a command or shortcut to do the on-demand "deep" collapse or expand, it's like toggling the jest.autoExpandSidebar settings, so developers can quickly zoom in/out dynamically.

I believe this is more for vscode than vscode-jest. But later we can add some button to the top like mocha extension does. Again good enough for first version.

in the explorer view, the file in the active editor gained autofocus; is it possible to do the same with the test sidebar? at least scroll/expand to the file level if not the test level by cursor...

You can go to the test or even line of the test failure already with a double-click.

gstamac · 2018-10-05T15:02:22Z

Can somebody review this again. Thanks.

connectdotz · 2018-10-06T04:17:44Z

@gstamac Thanks for following up , sorry for the delayed response, fell sick last 2 weeks...

I see the partial update is fixed, awesome! 👍

can we have a command or shortcut to do the on-demand "deep" collapse or expand, it's like toggling the jest.autoExpandSidebar settings, so developers can quickly zoom in/out dynamically.

I believe this is more for vscode than vscode-jest. But later we can add some button to the top like mocha extension does. Again good enough for first version.

that's fine.

I saw you now display the file nodes with the relative path, which is definitely shorter, but it could still truncate the actual file name. I wonder, instead of a flat file-path structure, how about a hierarchical one?

I don't agree with this. If you have a deep tree structure this will look terrible. I think this is OK for first version.

A hierarchical tree structure is the standard way vscode display files (see Explore view), while UI is objective, I don't think vscode Explore look terrible, quite the contrary IMHO 😏. UI aside, the biggest problem I have with flat mode is that it hides the most important information - filename:

(a snapshot from vscode-jest itself)

in the explorer view, the file in the active editor gained autofocus; is it possible to do the same with the test sidebar? at least scroll/expand to the file level if not the test level by cursor...

You can go to the test or even line of the test failure already with a double-click.

I think you are thinking from the sidebar to file, but I meant the other direction. From the test file, I would like to see the sidebar scroll to the active file accordingly.

please also take a look of the comments earlier regarding setting changes not get reflected, you would need to add the new settings in triggerUpdateSettings.

I will take a more indepth look at the code once we settle the discussion above. Thanks for your patient.

gstamac · 2018-10-06T11:46:42Z

I've implemented settings update on triggerUpdateSettings.

A hierarchical tree structure is the standard way vscode display files (see Explore view), while UI is objective, I don't think vscode Explore look terrible, quite the contrary IMHO 😏. UI aside, the biggest problem I have with flat mode is that it hides the most important information - filename:

Regarding file structure I understand your issues but I believe it shouldn't be used anyway. For tests there is another tree structure defined with describe blocks and I believe this should be the tree displayed in the sidebar. If you look at Mocha sidebar it doesn't have a file tree structure. I implemented it per your request but I still think it shouldn't be used. If somebody has a need for it and wants to add features to it they can do that.

I think you are thinking from the sidebar to file, but I meant the other direction. From the test file, I would like to see the sidebar scroll to the active file accordingly.

I think this is also related to the last request you had. I would see a better feature to scroll to the test you're on but I'm afraid this is too difficult to do with the current API. It would be too time intensive and it would make editing impossible. There is a feature request for VSCode (#5455) which would make it possible to click on the gutter decorator and go to the test. I believe this would be a much better solution.

connectdotz · 2018-10-07T02:38:07Z

Regarding file structure I understand your issues but I believe it shouldn't be used anyway. For tests there is another tree structure defined with describe blocks and I believe this should be the tree displayed in the sidebar.

As a tool provider, we don't get to decide users' test structure or practice. You might have a strong preference and good reason for having a root describe block, it doesn't mean other users' preferences or reasons are less valid. Actually, if you look at a few popular open source projects, you will see many of their tests do not have root describes: jest, yarn, or the default react apps generated by CRA. For projects without root describe blocks, the test file is its scope, equivalent to the root describe, therefore should be displayed accordingly; and the most intuitive UI is to adopt vscode's standard hierarchical tree struture that the Explore view used.

A good tool adapts to users environment, not the other way around. I am not arguing which test structure is better, only to emphasize that, as a tool provider, we need to support both. Having a setting showFilesInSidebar as you implemented is a great way to do this: for projects using root describes, they could simply turn off showFilesInSidebar and you got mocha like display; for the others, turn on showFilesInSidebar. Now, we just need to make sure the files do display clearly and intuitively when the flag is on.

I would see a better feature to scroll to the test you're on but I'm afraid this is too difficult to do with the current API...There is a feature request for VSCode (#5455) which would make it possible to click on the gutter decorator and go to the test.

indeed it would be nice if we can scroll to the particular test when clicked, but looks like VSCode (#5455) has been sitting for over 2 years, i.e. unlikely to be implemented soon, I think the next best thing is to scoll and expand the file/root-describe when active doc changed, once the test level event is available, it can be easily enhanced.

JextExt has a triggerUpdateActiveEditor to handle active editor change event, you can simply add the following tree view code there. Reading up the vscode api, it seems that you can get a TreeView by using createTreeView instead of registerTreeDataProvider; once you have the TreeView instance, you can scroll to the position with reval; and of course you already know how to do item expansion. Thus it seems relatively simple, but I haven't implemented it myself so could be wrong...

connectdotz

hey @gstamac your code is neat, I like it.

Let's do this, if you didn't have time to implement the file => treeView sync, we can table it for later. But I think the file display issue needs to be addressed. If you agree, we can finalize this PR once the change is in.

connectdotz · 2018-10-07T08:37:42Z

package.json

+          "type": "boolean",
+          "default": true
+        },
+        "jest.autoExpandSidebar": {


vscode displays settings in alphabetic order, thus the 2 sidebar variables don't appear together in the setting editor. Maybe you can rename these 2 identifiers (put sidebar up front?).

smith · 2018-12-11T06:44:28Z

@gstamac let me know if there's anything I can do to help get this finished up. Thanks for adding it!

gstamac · 2019-02-07T09:09:52Z

@smith Sorry haven't had time to work on this for a while. I renamed the config names. File display issue is not important in my opinion because that's not how it should be done anyway. If you look at Mocha addon it doesn't even support filename display. If you believe it's not done correctly I can remove it.

gstamac · 2019-03-11T11:16:08Z

@smith Have you had time to check this?

smith · 2019-03-11T13:28:46Z

@gstamac no it's kind of fallen of my radar for now. Sorry.

connectdotz · 2019-04-12T21:17:05Z

@gstamac we are preparing for the next major release that will incorporate multi-root and hopefully sidebar support too. see #442

I was trying to merge this PR to beta but multi-root has introduced some structural change that I figure you might want to do the merge yourself. It would be great if you can rebase this PR against beta branch, then I will merge it and help to get this feature out for the next release.

gstamac · 2019-04-17T10:16:07Z

@connectdotz I'll try to do it this weekend.

escaton · 2019-06-16T14:37:30Z

@gstamac do you need any help with rebase?

rhalaly · 2019-12-25T08:20:28Z

The images show that the test result indicator implemented as a circle with color indicator: green (pass) / red (failed).
But in this PR, they changed the indicator to an icon (✘, ✔) for colorblind people.

I think that this feature should also use the icon indicator for colorblind people and for uniform look & feel...

gerich-home · 2020-02-01T06:28:37Z

Hey guys, I see last comment in this PR on December. Any chance I could help with finishing this PR?
What should be done? (any checklist?)

connectdotz · 2020-02-02T16:46:03Z

@gerich-home If I remember correctly, it was last discussed that we should use vscode's standard hierarchical tree structure (like the Explore view) when showFilesInSidebar is true`. Would be great if we can get this completed 👍

wmertens · 2020-05-08T08:17:11Z

The sidebar view is super nice, it gives way clearer information than the console window. Would it not be an option to merge this as-is, and add the file tree later?
There are some other sidebar extensions, but they don't have all the other integrations that vscode-jest has.

rossknudsen · 2020-09-28T20:09:52Z

Just as a heads up - I've raised #625 which makes the test results from this extension available to other extensions. The aim being to enable an extension that I maintain to have access to the test results so they can be displayed in a side bar using Vs Code Test Explorer. It might be desirable to not merge this and not reinvent the wheel.

Either way, feel free to comment on #625 as I'm pretty keen to get it across the line. I've done some initial spike work with my extension and it seems to be able to access vscode-jest and get updates when the test results change.

connectdotz · 2021-07-03T20:13:45Z

close this in favor of the upcoming vscode testing API/Explorer. @gstamac sorry we did not reach the action plan sooner but thanks for the contribution nevertheless.

gstamac added 4 commits August 2, 2018 13:25

first version of sidebar implemented

c3276aa

open test file on node click

4e40c11

cache test file parsing for diagnostics

50e0088

renamed sidebar icons

039dae9

gstamac mentioned this pull request Aug 6, 2018

Use new test action bar panel to show test results and overview of all tests. #321

Closed

gstamac added 3 commits August 6, 2018 16:44

Merge remote-tracking branch 'upstream/master'

15d9fd2

# Conflicts: # package.json # src/JestExt.ts # src/Settings/index.ts # src/diagnostics.ts # src/extension.ts # tests/JestExt.test.ts # tests/diagnostics.test.ts # tests/extension.test.ts

removed orig files from merge

2732054

Merge remote-tracking branch 'upstream/master'

b0df658

connectdotz self-requested a review August 17, 2018 21:16

gstamac force-pushed the master branch from 1f4004d to 35c3994 Compare August 20, 2018 10:54

gstamac force-pushed the master branch from 35c3994 to 3ecffe1 Compare August 20, 2018 11:42

fixed tests and merge errors

d86f461

gstamac force-pushed the master branch from 3ecffe1 to d86f461 Compare August 20, 2018 12:45

gstamac added 4 commits August 20, 2018 15:04

use verbose for CI tests

67d3e2b

removed console.log from JestExt.test.ts

34ecbef

added changelog

40eec87

removed verbose for CI tests

2d19ca3

connectdotz requested changes Aug 21, 2018

View reviewed changes

gstamac added 4 commits August 23, 2018 17:00

added support for tests without describe

084c4e1

changed default settings for sidebar

cce9684

fixed settings tests

bb78173

try to fix Travis CI with --maxWorkers=4

4f9ad71

connectdotz requested changes Aug 25, 2018

View reviewed changes

gstamac added 2 commits September 24, 2018 14:42

Merge remote-tracking branch 'upstream/master'

7ccdd30

# Conflicts: # CHANGELOG.md

sidebar update on partial test run

f576f45

fixed "== should be ==="

9ea3637

listen to settings update

832c1f6

connectdotz reviewed Oct 7, 2018

View reviewed changes

rename sidebar config names

02ec9ad

connectdotz mentioned this pull request Apr 12, 2019

next release #442

Closed

12 tasks

rossknudsen mentioned this pull request Oct 3, 2020

Provide public API to subscribe to test results. #625

Closed

connectdotz closed this Jul 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sidebar support #357

Sidebar support #357

gstamac commented Aug 6, 2018 •

edited

Loading

gstamac commented Aug 14, 2018

connectdotz commented Aug 17, 2018

coveralls commented Aug 20, 2018 •

edited

Loading

gstamac commented Aug 20, 2018

connectdotz left a comment •

edited

Loading

connectdotz Aug 21, 2018

connectdotz Aug 21, 2018 •

edited

Loading

gstamac Aug 21, 2018

connectdotz Aug 21, 2018 •

edited

Loading

gstamac Aug 23, 2018

connectdotz Aug 25, 2018

connectdotz left a comment

gstamac commented Sep 25, 2018

gstamac commented Oct 5, 2018

connectdotz commented Oct 6, 2018

gstamac commented Oct 6, 2018 •

edited

Loading

connectdotz commented Oct 7, 2018 •

edited

Loading

connectdotz left a comment

connectdotz Oct 7, 2018

smith commented Dec 11, 2018

gstamac commented Feb 7, 2019

gstamac commented Mar 11, 2019

smith commented Mar 11, 2019

connectdotz commented Apr 12, 2019

gstamac commented Apr 17, 2019

escaton commented Jun 16, 2019

rhalaly commented Dec 25, 2019

gerich-home commented Feb 1, 2020

connectdotz commented Feb 2, 2020

wmertens commented May 8, 2020

rossknudsen commented Sep 28, 2020

connectdotz commented Jul 3, 2021

Sidebar support #357

Sidebar support #357

Conversation

gstamac commented Aug 6, 2018 • edited Loading

gstamac commented Aug 14, 2018

connectdotz commented Aug 17, 2018

coveralls commented Aug 20, 2018 • edited Loading

Pull Request Test Coverage Report for Build 488

💛 - Coveralls

gstamac commented Aug 20, 2018

connectdotz left a comment • edited Loading

Choose a reason for hiding this comment

connectdotz Aug 21, 2018

Choose a reason for hiding this comment

connectdotz Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

gstamac Aug 21, 2018

Choose a reason for hiding this comment

connectdotz Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

gstamac Aug 23, 2018

Choose a reason for hiding this comment

connectdotz Aug 25, 2018

Choose a reason for hiding this comment

connectdotz left a comment

Choose a reason for hiding this comment

gstamac commented Sep 25, 2018

gstamac commented Oct 5, 2018

connectdotz commented Oct 6, 2018

gstamac commented Oct 6, 2018 • edited Loading

connectdotz commented Oct 7, 2018 • edited Loading

connectdotz left a comment

Choose a reason for hiding this comment

connectdotz Oct 7, 2018

Choose a reason for hiding this comment

smith commented Dec 11, 2018

gstamac commented Feb 7, 2019

gstamac commented Mar 11, 2019

smith commented Mar 11, 2019

connectdotz commented Apr 12, 2019

gstamac commented Apr 17, 2019

escaton commented Jun 16, 2019

rhalaly commented Dec 25, 2019

gerich-home commented Feb 1, 2020

connectdotz commented Feb 2, 2020

wmertens commented May 8, 2020

rossknudsen commented Sep 28, 2020

connectdotz commented Jul 3, 2021

gstamac commented Aug 6, 2018 •

edited

Loading

coveralls commented Aug 20, 2018 •

edited

Loading

connectdotz left a comment •

edited

Loading

connectdotz Aug 21, 2018 •

edited

Loading

connectdotz Aug 21, 2018 •

edited

Loading

gstamac commented Oct 6, 2018 •

edited

Loading

connectdotz commented Oct 7, 2018 •

edited

Loading