Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Absolute coverage thresholds #7056

Closed
4 tasks done
jonahkagan opened this issue Dec 9, 2024 · 2 comments · Fixed by #7061
Closed
4 tasks done

Absolute coverage thresholds #7056

jonahkagan opened this issue Dec 9, 2024 · 2 comments · Fixed by #7061
Labels
feat: coverage Issues and PRs related to the coverage feature p2-nice-to-have Not breaking anything but nice to have (priority) pr welcome

Comments

@jonahkagan
Copy link
Contributor

Clear and concise description of the problem

As a developer using Vitest, I want a way to specify absolute coverage thresholds instead of percentage coverage thresholds. The benefit of absolute thresholds is that you can freeze the code coverage at a specific number of uncovered lines, which then ensures that test coverage must be considered for any new additions to the code.

For example, let's say I start working on a package that has 1000 lines, 750 of which are covered (75% coverage), so I set the coverage threshold to 75% to "preserve" this amount of coverage. I then add a new feature with 10 lines, 8 of which are covered. The new coverage percentage is 75.05%, so I pass coverage. However, if only 7 lines of the new feature are covered, then the new coverage percentage is 74.95%, and I fail. This feels a bit arbitrary - I've only enforced that new code additions must have as good of coverage as the existing code.

With absolute coverage thresholds, I would specify my coverage threshold as "250 uncovered lines." When adding a new feature with 10 lines, all 10 lines must be covered (or marked to be ignored in coverage).

In my organization, our approach to code coverage is to strive for 100% coverage unless there's a reasonable tradeoff (e.g. a particular is too hard to reproduce in testing to make it worthwhile, in which case we explicitly mark that case to be ignored). This is easy to do for code that already has 100% coverage. But for any legacy code that doesn't, we can't really implement this policy very well without absolute coverage thresholds.

We are currently migrating from jest (which supports this feature) to vitest. We are loving vitest but are sad to lose this important piece of functionality.

Suggested solution

Jest supports configuring a coverage threshold with a positive number, in which case its interpreted as a percentage, or a negative number, in which case its interpreted as an absolute threshold.

The advantage of this approach is that it doesn't add any new configuration options. The disadvantage is that it's not totally clear what's happening unless you read the docs.

I think it would also make sense to have a more explicit configuration option to use absolute or percentage thresholds in the coverage config. E.g. something like coverage.thresholdType: 'percentage' | 'absolute'.

I haven't yet looked into how to implement this in the code base, but I'd be happy to take the lead on researching and implementing the feature once its agreed that it would be a useful addition.

Alternative

No response

Additional context

Thank you for your time and consideration!

Validations

@AriPerkkio AriPerkkio added pr welcome feat: coverage Issues and PRs related to the coverage feature p2-nice-to-have Not breaking anything but nice to have (priority) and removed enhancement: pending triage labels Dec 10, 2024
@AriPerkkio
Copy link
Member

The disadvantage is that it's not totally clear what's happening unless you read the docs.

I think this is the reason why Vitest doesn't support it. And also nyc and c8 don't support it. But we can add it to Vitest just to be compatible with Jest.

I think using the negative numbers is fine, but if you can come up with better API we could add that too.

I haven't yet looked into how to implement this in the code base, but I'd be happy to take the lead on researching and implementing the feature once its agreed that it would be a useful addition.

All coverage threholds related logic is in packages/vitest/src/utils/coverage.ts. Comparison is done here, though I think we need to consider negative numbers in thresholds.autoUpdate related logic too. The summary.data[thresholdKey] might hold info about uncovered lines already.

/**
* Check collected coverage against configured thresholds. Sets exit code to 1 when thresholds not reached.
*/
private checkThresholds(allThresholds: ResolvedThreshold[]) {
for (const { coverageMap, thresholds, name } of allThresholds) {
if (
thresholds.branches === undefined
&& thresholds.functions === undefined
&& thresholds.lines === undefined
&& thresholds.statements === undefined
) {
continue
}
// Construct list of coverage summaries where thresholds are compared against
const summaries = this.options.thresholds?.perFile
? coverageMap.files().map((file: string) => ({
file,
summary: coverageMap.fileCoverageFor(file).toSummary(),
}))
: [{ file: null, summary: coverageMap.getCoverageSummary() }]
// Check thresholds of each summary
for (const { summary, file } of summaries) {
for (const thresholdKey of THRESHOLD_KEYS) {
const threshold = thresholds[thresholdKey]
if (threshold !== undefined) {
const coverage = summary.data[thresholdKey].pct
if (coverage < threshold) {
process.exitCode = 1
/*
* Generate error message based on perFile flag:
* - ERROR: Coverage for statements (33.33%) does not meet threshold (85%) for src/math.ts
* - ERROR: Coverage for statements (50%) does not meet global threshold (85%)
*/
let errorMessage = `ERROR: Coverage for ${thresholdKey} (${coverage}%) does not meet ${
name === GLOBAL_THRESHOLDS_KEY ? name : `"${name}"`
} threshold (${threshold}%)`
if (this.options.thresholds?.perFile && file) {
errorMessage += ` for ${relative('./', file).replace(/\\/g, '/')}`
}
this.ctx.logger.error(errorMessage)
}
}
}
}
}
}

Example test case:

import { expect } from 'vitest'
import { sum } from '../fixtures/src/math'
import { coverageTest, isV8Provider, normalizeURL, runVitest, test } from '../utils'
test('failing thresholds', async () => {
const { exitCode, stderr } = await runVitest({
include: [normalizeURL(import.meta.url)],
coverage: {
all: false,
include: ['**/fixtures/src/math.ts'],
thresholds: {
'**/fixtures/src/math.ts': {
branches: 100,
functions: 100,
lines: 100,
statements: 100,
},
},
},
}, { throwOnError: false })
const lines = isV8Provider() ? '50%' : '25%'
const statements = isV8Provider() ? '50%' : '25%'
expect(exitCode).toBe(1)
expect(stderr).toContain(`ERROR: Coverage for lines (${lines}) does not meet "**/fixtures/src/math.ts" threshold (100%)`)
expect(stderr).toContain(`ERROR: Coverage for statements (${statements}) does not meet "**/fixtures/src/math.ts" threshold (100%)`)
expect(stderr).toContain('ERROR: Coverage for functions (25%) does not meet "**/fixtures/src/math.ts" threshold (100%)')
})
coverageTest('cover some lines, but not too much', () => {
expect(sum(1, 2)).toBe(3)
})

@jonahkagan
Copy link
Contributor Author

Thanks for the pointers @AriPerkkio - very helpful! I submitted a pull request here: #7061.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: coverage Issues and PRs related to the coverage feature p2-nice-to-have Not breaking anything but nice to have (priority) pr welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants