fix(workspace): throw error when browser mode and `@vitest/coverage-v8` are used by AriPerkkio · Pull Request #5250 · vitest-dev/vitest

AriPerkkio · 2024-02-20T19:33:33Z

Description

This error is not thrown when browser mode is used with @vitest/coverage-v8. Browser tests start and show errors related node:inspector usage outside Node.

vitest/packages/vitest/src/node/config.ts

Lines 142 to 143 in 4ff7159

    
           if (resolved.coverage.provider === 'v8' && resolved.coverage.enabled && isBrowserEnabled(resolved)) 
        
             throw new Error('@vitest/coverage-v8 does not work with --browser. Use @vitest/coverage-istanbul instead')

The reason is that resolved configuration is for root configuration, not the workspace configuration. Coverage options are in root config, browser config in workspace config. At this scope we don't have access to those both I think.

// vitest.config.ts
import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    coverage: {
      enabled: true,
    },
  },
});

// vitest.workspace.ts
import { defineWorkspace } from "vitest/config";

export default defineWorkspace([
  {
    test: {
      name: "Browser project",
      browser: {
        headless: true,
        enabled: true,
        name: "chrome",
      },
    },
  },
]);

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
Ideally, include a test that fails without this PR but passes with it.
Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

Run the tests with pnpm test:ci.

Documentation

If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

netlify · 2024-02-20T19:33:49Z

✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name	Link
🔨 Latest commit	`e256152`
🔍 Latest deploy log	https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65dc218fa06ca300082975ae

packages/vitest/src/node/workspace.ts

hi-ogawa

Thanks for investigation. I see why resolveConfig doesn't throw for workspace browser and your approach looks good to me.

As an alternative, I'm wondering if we can move up getSerializableConfig's root config merging:

vitest/packages/vitest/src/node/workspace.ts

Lines 330 to 332 in 366a78b

    
           return deepMerge({ 
        
             ...this.config, 
        
             coverage: this.ctx.config.coverage,

to before resolveConfig:

vitest/packages/vitest/src/node/workspace.ts

Lines 299 to 300 in 366a78b

    
           async setServer(options: UserConfig, server: ViteDevServer) { 
        
             this.config = resolveConfig(this.ctx.mode, options, server.config)

Something like this?

this.config = resolveConfig(
  this.ctx.mode,
  {
    ...options,
    coverage: this.ctx.config.coverage,
  },
  server.config
);

I feel this is somewhat awkward, so I like doing checks during resolveWorkspace though.

…8` are used

AriPerkkio · 2024-02-26T05:31:53Z

Merging coverage options as early as in setServer's resolveConfig seems like the best option. Now we don't need to duplicate the logic of "browser with v8-coverage" checks.

The other configuration modifications done in getSerializableConfig are quite tricky as they may depend on configuration resolved in resolveConfig. Ideally the poolOptions should also be moved to setServer()'s scope but that doesn't seem possible now. I'll leave those as they were.

hi-ogawa

I see, throwing during WorkspaceProject.setServer propagates the error mostly same way, then that's good 👍

I didn't think about this approach when I was fixing workspace snapshot option #5199, but probably I could've done something similar.

packages/vitest/src/node/workspace.ts

AriPerkkio commented Feb 20, 2024

View reviewed changes

packages/vitest/src/node/workspace.ts Outdated Show resolved Hide resolved

AriPerkkio force-pushed the fix/browser-coverage-v8-show-error branch 4 times, most recently from 1551a3c to 28e077c Compare February 24, 2024 12:56

AriPerkkio marked this pull request as ready for review February 24, 2024 12:57

hi-ogawa reviewed Feb 26, 2024

View reviewed changes

fix(workspace): throw error when browser mode and `@vitest/coverage-v…

e256152

…8` are used

AriPerkkio force-pushed the fix/browser-coverage-v8-show-error branch from 28e077c to e256152 Compare February 26, 2024 05:28

hi-ogawa approved these changes Feb 26, 2024

View reviewed changes

sheremet-va approved these changes Feb 26, 2024

View reviewed changes

sheremet-va reviewed Feb 26, 2024

View reviewed changes

packages/vitest/src/node/workspace.ts Show resolved Hide resolved

sheremet-va merged commit 29f98cd into vitest-dev:main Feb 27, 2024

AriPerkkio deleted the fix/browser-coverage-v8-show-error branch February 27, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(workspace): throw error when browser mode and `@vitest/coverage-v8` are used#5250

fix(workspace): throw error when browser mode and `@vitest/coverage-v8` are used#5250
sheremet-va merged 1 commit intovitest-dev:mainfrom
AriPerkkio:fix/browser-coverage-v8-show-error

AriPerkkio commented Feb 20, 2024 •

edited

Loading

Uh oh!

netlify bot commented Feb 20, 2024 •

edited

Loading

Uh oh!

Uh oh!

hi-ogawa left a comment

Uh oh!

AriPerkkio commented Feb 26, 2024

Uh oh!

hi-ogawa left a comment

Uh oh!

Uh oh!

	if (resolved.coverage.provider === 'v8' && resolved.coverage.enabled && isBrowserEnabled(resolved))
	throw new Error('@vitest/coverage-v8 does not work with --browser. Use @vitest/coverage-istanbul instead')

	return deepMerge({
	...this.config,
	coverage: this.ctx.config.coverage,

	async setServer(options: UserConfig, server: ViteDevServer) {
	this.config = resolveConfig(this.ctx.mode, options, server.config)

Uh oh!

Comments

Conversation

AriPerkkio commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Documentation

Changesets

Uh oh!

netlify bot commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.

Uh oh!

Uh oh!

hi-ogawa left a comment

Choose a reason for hiding this comment

Uh oh!

AriPerkkio commented Feb 26, 2024

Uh oh!

hi-ogawa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AriPerkkio commented Feb 20, 2024 •

edited

Loading

netlify bot commented Feb 20, 2024 •

edited

Loading