feat: adding tests count badge for the testing explorer #182399
Conversation
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
|
@microsoft-github-policy-service agree |
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
|
I've added the badgeTooptip, so if 
|
src/vs/workbench/contrib/testing/browser/testingExplorerView.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: John Murray <johnm@georgejames.com>
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
| @@ -81,8 +82,10 @@ export class TestingExplorerView extends ViewPane { | |||
| private filterActionBar = this._register(new MutableDisposable()); | |||
| private container!: HTMLElement; | |||
| private treeHeader!: HTMLElement; | |||
| private countSummary!: CountSummary; | |||
There was a problem hiding this comment.
This isn't defined at a known point in the lifecycle. I would remove this property and just pass the summary as an argument to renderActivityCount
There was a problem hiding this comment.
Passing the summary as an argument won't make the badge reactive when there's a change in the configurations, that's why I thought about keeping the summary in the property, so whenever the configuration is updated the render method can find the summary.
this._register(onDidChangeTestingCountBadge(this.renderActivityCount, this));
But thinking about it, do we really want to make it that reactive? I mean, if we just need to react only if it went from any type to off, we can pass undefined maybe as an argument to the function and it would work. but if we want to make it totally reactive, like if you run the tests with badge type passed and then changed it to failed, what do you expect? do you expect to see the number updated immediately?
what do you think?
There was a problem hiding this comment.
Ah, I see, missed that usage. I would then just remove the ! and type it as CountSummary | undefined
There was a problem hiding this comment.
done, added a guard check also in the render method, are we fine with that?
| @@ -37,6 +38,14 @@ export const enum DefaultGutterClickAction { | |||
| ContextMenu = 'contextMenu', | |||
| } | |||
|
|
|||
| export const enum TestingCountBadge { | |||
| All = 'all', | |||
There was a problem hiding this comment.
I'm not a big fan of having "all" as an option. The purpose of the badges is to show things that are happening (busy state when committing on SCM), or things that need to be done (unsaved files in the explorer, or extensions that need updating). Tests that exist are neither of these things. The closest analog would be if the explorer showed a badge displaying the number of files in your workspace, which would be rather odd.
There was a problem hiding this comment.
Yes, it makes sense, I totally agree with you
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi Abdulfattah <iifawzie@gmail.com>
Hi, this PR should resolve [Feature request] Test explorer show failing tests number in the Activity Bar Badge
Screen.Recording.2023-05-13.at.7.28.54.PM.mp4