Browser mode: Istanbul coverage report incomplete

[lsim]

Describe the bug

The following test file causes trouble with vitest in browser mode (chrome). Notice the nested describe calls:

import { describe, it, expect } from 'vitest';
import { foo, bar, baz } from '../src/stuff';

describe('stuff 1', () => {
  it('should baz', () => {
    expect(baz()).toBe('doo');
  });

  describe('stuff 2', () => {
    describe('stuff 3', () => {
      it('should foo', () => {
        expect(foo()).toBe('bar');
      });
    });
    it('should bar', () => {
      expect(bar()).toBe('foo');
    });
  });
});

What seems to happen is that all three tests are run (and pass), but only the should baz and should foo count towards the coverage report. In the repro repo, this means line coverage is reported as 66%.

The issue only occurs when the tests are browser based. With the jsdom environment, things work as expected and the coverage is reported as 100%.

Reproduction

Reproduction repo is here

System Info

System:
    OS: macOS 13.4
    CPU: (10) arm64 Apple M1 Max
    Memory: 1.84 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.16.0 - ~/.nvm/versions/node/v18.16.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.16.0/bin/yarn
    npm: 9.5.1 - ~/.nvm/versions/node/v18.16.0/bin/npm
  Browsers:
    Chrome: 113.0.5672.126
    Firefox: 107.0.1
    Safari: 16.5
  npmPackages:
    @vitest/browser: ^0.31.1 => 0.31.4
    @vitest/coverage-istanbul: ^0.31.4 => 0.31.4
    @vitest/ui: ^0.31.1 => 0.31.4
    vite: ^4.3.2 => 4.3.9
    vitest: ^0.31.1 => 0.31.4

Used Package Manager

yarn

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.

[elby22]

@lsim are you using the browser.slowHijackESM config option? I also had this issue, and when I was digging into it I found that the source maps weren't accounting for the transformations done with that option.

[lsim]

Thanks for your input! It's something to look into.

That option is on by default and I hadn't disabled it. So yes - I am using that. If I'm reading the docs right, I don't need it. Disabling it stops all my browser tests from running though.

No errors, no output.

I'll do a bit more experimenting

[lsim]

Not sure if it's helpful, but with browser.slowHijackESM disabled, I get the following in the console of the browser used for running the tests:

[lsim]

Weirdly with browser.headless = false, my tests fail with

TypeError: Cannot redefine property: chrome

Presumably from this line:

vi.stubGlobal('chrome', chrome);

Same tests work when running in headless chrome. This endeavour is a bit like walking blindfolded through a mine field :)

I guess for now I'll settle for the path that gives me working tests and broken coverage metrics.

[elby22]

I can't get my tests to run without the browser.slowHijackESM option either, so I'm in the same boat.

It seems like the instrumented code that the test is running on is mapping incorrectly to the source code. In packages/coverage-istanbul/src/provider.ts, onFileTransform does seem to be using the getCombinedSourcemap from the rollup plugin, but the mappings provided with the instrumented version of the file is still wrong.

Here is an example

[sheremet-va]

I can't get my tests to run without the browser.slowHijackESM option either, so I'm in the same boat.

This should be fixed in 0.32.1

It seems like the instrumented code that the test is running on is mapping incorrectly to the source code. In packages/coverage-istanbul/src/provider.ts, onFileTransform does seem to be using the getCombinedSourcemap from the rollup plugin, but the mappings provided with the instrumented version of the file is still wrong.

Maybe we need to provide order: 'post' in the plugin so it runs after all transformations 🤔

[elby22]

@sheremet-va I added enforce: 'post' to the CoverageTransform plugin function and it did produce a different transformation, just not the correct one.
Here is the output to compare against my previous.

[elby22]

Also just updated to 0.32.2 with browser.slowHijackESM = false and the tests run. However I'm still getting incorrect coverage.

[elby22]

@lsim I explored this some more and I believe the issue is not due to coverage-istanbul but how the browser runner differs from the standard runner. Here is a reproduction to demonstrate .

@sheremet-va I'm happy to make a PR if you can point me in the right direction. Do tests for this belong in test/coverage-test/coverage-report-tests?

From the readme:

While debugging, I noticed that takeCoverage() in the coverage-istanbul package gets called multiple times in browser mode, and only once with node. It seems like the browser is calling onAfterRunSuite in packages/browser/src/client/runner.ts 4 times.

Removing globalThis[COVERAGE_STORE_KEY] = {} in packages/coverage-istanbul/src/index.ts: takeCoverage() does produce 100% coverage, but with higher counts for coverage hits.

So far I don't think there is an issue with the coverage-istanbul package, but with how the browser runner is handling suites. Running without any describe() blocks (find commented-out in the test file) gives 100% coverage for both.

[elby22]

@sheremet-va I've added a PR (#3806). First time contributing to this repo and I left some notes for the reviewer. Thanks.

[AriPerkkio]

@elby22 your analysis on the root cause sounds correct. The browser runner is using different hook when compared to NodeJS's worker_thread/child_process runner.

vitest/packages/browser/src/client/runner.ts

Lines 42 to 46 in 8693449

	async onAfterRunSuite() {
	await super.onAfterRunSuite?.()
	const coverage = await coverageModule?.takeCoverage?.()
	await rpc().onAfterSuiteRun({ coverage })
	}

vitest/packages/vitest/src/runtime/entry.ts

Lines 67 to 72 in 8693449

	const originalOnAfterRun = testRunner.onAfterRun
	testRunner.onAfterRun = async (files) => {
	const coverage = await takeCoverageInsideWorker(config.coverage, executor)
	rpc().onAfterSuiteRun({ coverage })
	await originalOnAfterRun?.call(testRunner, files)
	}

I think the rpc.onAfterSuiteRun is poorly named as it should actually be something like onAfterTestFileRun. But let's not change that at this point.

[AriPerkkio]

Fixed in #3806.