Support for non-global code coverage thresholds #18
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 69 81 +12
=========================================
+ Hits 69 81 +12
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This looks great, @thall90! Added some questions/suggestions.
In addition, I wonder if updating all declared threshold paths in the config file—instead of only global or those explicitly defined—would be a better default. What do you think?
| @@ -66,9 +72,17 @@ Options: | |||
| -c, --config <path> path to a Jest config file (default: 'jest.config.js') | |||
| -m, --margin <margin> minimum threshold increase (default: 0) | |||
| -t, --tolerance <tolerance> threshold difference from actual coverage | |||
| -k, --thresholdKeys <keys> comma-separated list of threshold keys to consider (default: 'global') | |||
There was a problem hiding this comment.
Jest calls these "paths", I think we should do the same for consistency. Also, we should probably use a variadic option:
| -k, --thresholdKeys <keys> comma-separated list of threshold keys to consider (default: 'global') | |
| -p, --threshold-path <paths...> comma-separated list of threshold keys to consider (default: 'global') |
| Object.entries(newValues).forEach(([type, { prev, next, diff }]) => { | ||
| const regex = new RegExp( | ||
| `(?<=${keyPrefix}${thresholdKey}:\\s*{[^}]*${type}:\\s*)\\d*\\.?\\d+(?=,\\s*\\n\\s*${ | ||
| type === 'statements' ? '}' : '' |
There was a problem hiding this comment.
Can we safely assume that statements always comes last?
|
|
||
| it('returns changes for an updated custom threshold', () => { | ||
| fs.readFileSync.mockReturnValue(` | ||
| module.exports = { |
There was a problem hiding this comment.
You can use the getContents helper with level2 = './src/modules/**/*.module.ts' to generate this mock data.
|
@thall90 are you going to complete it? |
|
@rbardini @jimmywarting @vvscode I apologize for the delay - life's been busy, to say the least. @rbardini I agree that bumping up coverage for all threshold paths by default makes sense. I'll have that change along with all of the requested changes pushed tomorrow 👍 |
|
@rbardini While wrapping up changes to this PR, I found what appears to be a pretty big miss in my original changes: In My belief is that we will need to implement similar functionality to Jest's coverage threshold checking in order to verify whether a given threshold path's values should be bumped, since Jest doesn't appear to expose the result of their threshold checks. Happy to move forward with implementing these changes as well, but I wanted to get your take on the problem and the approach first. Thanks in advance! |
|
I just implemented some workaround script for my project and it works in a way
|
This PR adds support for non-global code coverage thresholds in the form of
threshold-keys, which should resolve #13.