Coverage report missing for untested files

[Idlesome]

Describe the bug

When running a coverage report using instanbul or c8, modules that aren't tested aren't being shown in the report. This could mean I have a partially tested codebase but still get a 100% code coverage report. This could be particularly troublesome if you have a large codebase and a small number of files are not tested (e.g. after a refactor). These could be quite easily missed, while the user sees 100% code coverage.
In jest, this is usually solved with the "collectCoverageFrom" option which allows for specifying a pattern to match files that should be analysed for coverage. Perhaps a similar setting could be introduced.

Note: ErrorBoundary and FallbackComponent are used in the application but not imported by any .test.* module

Reproduction

To reproduce, run a coverage report using vitest with a module that is tested and a module that is not. The untested module does not appear in the coverage report, but it should as it may still be used by the application. This can lead to false coverage reports.
If further reproduction is required, I'm happy to submit a minimal reproduction sometime soon but need sleep right now..!

System Info

N/A

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[xsjcTony]

I think you might need to add all: true into the coverage configuration, also as well as properly setup your include option in the coverage

[Idlesome]

Hi, thanks so much for your comment! You're totally right, just adding all: true to coverage in my vite.config.js fixed it (though I imagine include would be more useful when I have files that I want to exclude).

I should have noticed there was a "Config Reference" page but for some reason (tiredness probably!) I missed that. I think I was expecting the coverage guide to mention this somewhere. I've created a PR to add a link, in case that's useful.

Is all: false the best default? If we're going to collect coverage from only tested files by default, then coverage is always going to be reported as fairly high and it would be easy to miss that some files are not being tested. I think as a default it would be better to include all files until the user chooses to exclude certain patterns, or set all: false and set an include pattern.

Either that, or do what Jest does (though I'm not a fan of it): generating an empty report until a user adds files using collectCoverageFrom.

Open to thoughts/scrutiny and happy to help in any way I can :)

[AriPerkkio]

Is all: false the best default?

Maybe not. Currently Vitest exposes many coverage options in its API - maybe even too many. At some point it should be checked whether some of the options need to be configurable at all.

The default value for coverage.all is identical with nyc and c8:

• https://github.com/istanbuljs/nyc#common-configuration-options
• https://github.com/bcoe/c8#cli-options--configuration

Jest's collectCoverageFrom is like hard-coded all: true, where the include and exclude do the filtering. Personally I like that approach better.

[Idlesome]

I see - well it does make sense to keep coverage.all the same default as used by nyc and c8 for consistency. Although all: false doesn't seem that useful to someone who wants to see coverage for files other than those touched by tests (I would expect this to be everyone). From my testing, it seems that include has no effect when all is set to false.

Jest's collectCoverageFrom is like hard-coded all: true, where the include and exclude do the filtering. Personally I like that approach better.

Yeah, actually I agree - perhaps this should be the recommended approach. Something like this would work:

{
  ...
  coverage: {
    // Instrument all files
    all: true,
    // Optional - exclude generated source code from coverage reports
    exclude : [
      "**/*.generated.ts"
    ]
  }
}

Note, we don't need to set include because it's default is **. However for more control, we could override it:

{
  ...
  coverage: {
    // Instrument all files
    all: true,
    // Include only .ts files
+    include : [
+      "**/*.ts"
+    ],
    // Optional - exclude generated source code from coverage reports
    exclude : [
      "**/*.generated.ts"
    ]
  }
}

Given that all: false doesn't seem that useful, I think there are two options here:

1. Update documentation to recommend always configuring all: true
2. Set all: true as the default for vitest so users get full coverage reports out of the box

The decision for this comes down to whether you want users to be familiar with the defaults of the underlying coverage providers and their caveats or whether you want things to "just work" out of the box. I'd personally lean towards the 2nd option, as it feels wrong to tell users to configure vitest to report coverage for all of their files - they'd expect it to do this anyway. Again, very happy to be corrected! :)

[AriPerkkio]

From my testing, it seems that include has no effect when all is set to false.

The coverage.include is used against the files that are covered by tests. The default [**] means that all files imported by tests will be instrumented. Changing that value to something else will exclude those files from coverage report. The all is used to pick 100% uncovered files only. The name all may seem misleading - it's more like "include files that were not imported by tests but match includes in this working directory". 😄

There's a good summary in nyc project describing how the filtering works. Same methods should apply to Vitest, mostly. https://github.com/istanbuljs/nyc#selecting-files-for-coverage

Given that all: false doesn't seem that useful, [...]

To make things easier for end users, I think we could remove all flag from configuration options completely and internally set it as true. This would be a breaking change of course. At the same time we should do some more pruning to other coverage options as well and see if there's more options we could remove. For example the coverage.excludeNodeModules should be completely removed.

[Idlesome]

The name all may seem misleading - it's more like "include files that were not imported by tests but match includes in this working directory". 😄

Ahh yes, that makes sense!

To make things easier for end users, I think we could remove all flag from configuration options completely and internally set it as true [...]

So I guess this should be as simple as:
https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/defaults.ts#L28

const coverageConfigDefaults = {
-  all: false,
+  all: true,  

At the same time we should do some more pruning to other coverage options as well and see if there's more options we could remove. For example the coverage.excludeNodeModules should be completely removed.

When you say coverage.excludeNodeModules should be completely removed, do you mean the end user shouldn't be able to set it? So we'd set it to true internally, but not expose it as a configurable option?

I think there would be most value by initially:

• Setting all to true
• Major version change as you suggest
• An update to the docs (default for all would then be true)

The other options could be reviewed separately? Though I do agree they should be reviewed

[AriPerkkio]

When you say coverage.excludeNodeModules should be completely removed, do you mean the end user shouldn't be able to set it? So we'd set it to true internally, but not expose it as a configurable option?

Yes, exactly. There are also other configuration options that could be improved.

I think there would be most value by initially:

• Setting all to true
• [...]

The other options could be reviewed separately?

Sure, for now this change could be its own PR. Later when going through other options, we might end up removing all from configuration options completely and rely on includes + excludes combination instead. But that kind of changes required some API designing and manual testing, so it might take some time.

[madeleineostoja]

One major issue in defaulting all: true is that C8 has a longstanding bug where ignore comments don't work when running --all, so it'll be broken out the box until it's fixed upstream bcoe/c8#318

[AriPerkkio]

@madeleineostoja that c8 issue was caused by a dependency which is now fixed: istanbuljs/v8-to-istanbul#217.