Enables a hit count indicator to show number of times a line has been hit #491
Conversation
| renderOptions: { | ||
| before: { | ||
| // We use this special invisible character for the margin since spaces are trimmed | ||
| contentText: '\u00A0'.repeat(paddingWidth) + '\u00A0', |
There was a problem hiding this comment.
Ahh this is crafty, nice find / work around!
| const showLineCoverage = rootConfig.get("showLineCoverage") as string; | ||
| const showRulerCoverage = rootConfig.get("showRulerCoverage") as string; | ||
| this.showHitCounts = rootConfig.get("showHitCounts") as boolean; | ||
| const hitCountColor = rootConfig.get("hitCountColor") as string; |
There was a problem hiding this comment.
Should the colour be coupled to the gutter / line colour? Or is there a good reason that the hit count could be colourized 🤔.
There was a problem hiding this comment.
Not really, it was mostly just easier and more flexible that way. But coupling it to the gutter or line colour also makes sense
So just to confirm, are you suggesting that the hitCountColor should match the line colour (maybe a bit darker)? Or would you prefer the approach where we hardcode it, white for dark themes and black for light themes? Which one do you think works better? (or leave it as is and people can choose)
There was a problem hiding this comment.
We can leave it as is and let people choose, but we will need a default and that could be white for dark themes and black for light themes.
(if we don't want dynamic default based on the currently active colour theme, I guess we can figure out something that works for both light / dark themes?)
| "coverage-gutters.showHitCounts": { | ||
| "type": "boolean", | ||
| "scope": "resource", | ||
| "default": false, |
There was a problem hiding this comment.
This feature is pretty sweet (imo), it would be nice to have on my default, but I am not sure if developers would immediately "get" what it was for 🤔.
@mattseddon and I chatted about small tutorials or toasts that prompt these kinda features for users to enable but we never got around to building any of that out...
There was a problem hiding this comment.
Thanks! Yes, I totally agree that it might be confusing for newcomers. Having some sort of intro is a good idea imo. before enabling this feature by default
On the other hand, I have nothing against enabling it by default now
There was a problem hiding this comment.
On the other hand, I have nothing against enabling it by default now
Let's leave it off for the first release, and then we can turn it on for all in the one after. We could also consider maybe a tutorial / mention in the main readme.md showing off the feature (currently the gif / image is pretty out of date and could use a new short highlight walkthrough).
src/coverage-system/renderer.ts
Outdated
| } | ||
|
|
||
| const range = new Range(lineNumber, 0, lineNumber, 0); | ||
| const paddedHitCount = hitCount.toString().padStart(paddingWidth, '\u00A0'); |
There was a problem hiding this comment.
I had a thought while cycling home (around the max hit count), we could probably add an if check here and use something like >1000 as the default value if the hitCount reaches something above that 🤔.
Let me know your thoughs as well @ogechno .
There was a problem hiding this comment.
Totally makes sense, thanks for the feedback! I will add this in the coming days
There was a problem hiding this comment.
Just done it now quickly :) let me know what you think
There was a problem hiding this comment.
Sweet thanks, looking great now.
|
@ogechno I think the only remaining item is to figure out the test failures + maybe add one or two new test cases. (I might have some time today / tomorrow to contribute to that effect). |
2 participants
This feature enables users to view the amount of times a line has been hit during test execution. This closes the discussion in issue #194
Design Rationale
In issue #194, there was the discussion around having a GitLens-style hit counter at the end of each line. I decided against that approach since usually you want to quickly skim the coverage report and spot any lines with low hit counts during execution (e.g. when reviewing fuzzing coverage reports). This would be difficult if the hit counter is at the end of the line.
That's why I went with displaying the hit counts to the left of the code, similar to how lcov and grcov are doing it. This makes it much easier to scan through the report and immediately see which lines might need more test coverage, rather than having to read across each line to find the counts at the end
Screenshot