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

UserConfigExport['coverage']['provider'] with a object option did not work at all #2797

Closed
6 tasks done
mmis1000 opened this issue Feb 3, 2023 · 8 comments · Fixed by #2817
Closed
6 tasks done

UserConfigExport['coverage']['provider'] with a object option did not work at all #2797

mmis1000 opened this issue Feb 3, 2023 · 8 comments · Fixed by #2817
Labels
feat: coverage Issues and PRs related to the coverage feature

Comments

@mmis1000
Copy link

mmis1000 commented Feb 3, 2023

Describe the bug

When you defined setting as following

const CoverageModule = {
  getProvider: () => { throw 'called' }
}
export default defineConfig({
  test: {
    coverage: {
      provider: CoverageModule
    }
  }
})

It always say your provider is c8

Note: it is a bug in

if (isMergeableObject(source[key])) {
if (!target[key])
target[key] = {} as any
deepMerge(target[key] as any, source[key] as any)
}

Condition here do no check for isMergeableObject(target[key]) .
With in this case, cause deepMerge({ provider: 'c8' }, { provider: CoverageModule }) to result in { provider: 'c8' }

Yet another note:

It would still crash even this fixed because it is unable to send this object to other worker.
This option should probably not be the module instance at first place.

圖片

Yet Yet another note:

It seems worker is used even I run it with vitest run --no-threads --coverage --no-isolate, this can not be right...

Reproduction

Run npx vitest --run --coverage in https://stackblitz.com/edit/vitest-dev-vitest-x4h2tg?file=vite.config.ts

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
  npmPackages:
    @vitest/ui: latest => 0.28.3 
    vite: latest => 4.1.1 
    vitest: latest => 0.28.3

Used Package Manager

npm

Validations

@mmis1000 mmis1000 changed the title UserConfigExport['coverage']['provider'] with a object option did not work at all UserConfigExport['coverage']['provider'] with a object option did not work at all Feb 3, 2023
@sheremet-va sheremet-va added the bug label Feb 3, 2023
@AriPerkkio
Copy link
Member

Custom coverage providers have been broken for a while. 🙃
I've planned to track this bug for a while but other things got priority.

Condition here do no check for  isMergeableObject(target[key])

In addition to this bug there is another issue where either Tinypool or brpc throws DataClone errors. I haven't debugged that any further, yet.

@AriPerkkio AriPerkkio added the feat: coverage Issues and PRs related to the coverage feature label Feb 3, 2023
@sheremet-va
Copy link
Member

sheremet-va commented Feb 3, 2023

In addition to this bug there is another issue where either Tinypool or brpc throws DataClone errors. I haven't debugged that any further, yet.

It will throw this error if ctx.getSerializableConfig contains a function.

@AriPerkkio
Copy link
Member

@mmis1000 following seems to work on the reproduction case:

import { defineConfig } from 'vitest/config';
import { CoverageProvider, CoverageProviderModule } from 'vitest';

class CoverageModule implements CoverageProviderModule {
  getProvider(): CoverageProvider {
    throw 'called';
  }
};
export default defineConfig({
  test: {
    coverage: {
      enabled: true,
      provider: new CoverageModule(),
    },
  },
});
❯ npm test
$ vitest
called

I'm not sure when exactly this has been fixed - by accident. I've had some tests for custom reporters in separate feature branch and earlier those were not working at all. Now they seem to run nicely. I'll set up PR for those tests later.

Here's some draft for reference:

custom-coverage-provider.ts
import type { CoverageProvider, CoverageProviderModule, ResolvedCoverageOptions, Vitest } from 'vitest'

class CustomCoverageProvider implements CoverageProvider {
  name = 'custom-coverage-provider'

  ctx!: Vitest
  options!: ResolvedCoverageOptions

  initialize(ctx: Vitest) {
    this.ctx = ctx
    this.options = {} as ResolvedCoverageOptions
  }

  clean() {
    console.log('CustomProvider::clean')
  }

  onBeforeFilesRun() {
    console.log('CustomProvider::onBeforeFilesRun')
  }

  onAfterSuiteRun() {
    console.log('CustomProvider::onAfterSuiteRun')
  }

  reportCoverage() {
    console.log('CustomProvider::reportCoverage')
  }

  onFileTransform() {
    console.log('CustomProvider::onFileTransform')
  }

  resolveOptions() {
    return this.options
  }
}

export default class CustomCoverageProviderModule implements CoverageProviderModule {
  getProvider(): CoverageProvider {
    return new CustomCoverageProvider()
  }

  takeCoverage() {}
}
vitest.config.ts
import { defineConfig } from 'vitest/config'
import CustomCoverageProviderModule from './custom-coverage-provider'

export default defineConfig({
  test: {
    coverage: {
      enabled: true,
      provider: new CustomCoverageProviderModule(),
    },
  },
})
$ pnpm test

CustomProvider::clean

 RUN  v0.28.4 /Users/x/y/vitest/test/coverage-test
      Coverage enabled with custom-coverage-provider

CustomProvider::onBeforeFilesRun
CustomProvider::onFileTransform
CustomProvider::onFileTransform
...

 Test Files  3 passed (3)
      Tests  7 passed (7)
   Start at  12:03:47
   Duration  410ms (transform 159ms, setup 15ms, collect 133ms, tests 20ms)

 % Coverage report from custom-coverage-provider
CustomProvider::reportCoverage

@mmis1000
Copy link
Author

mmis1000 commented Feb 5, 2023

@mmis1000 following seems to work on the reproduction case:

import { defineConfig } from 'vitest/config';
import { CoverageProvider, CoverageProviderModule } from 'vitest';

class CoverageModule implements CoverageProviderModule {
  getProvider(): CoverageProvider {
    throw 'called';
  }
};
export default defineConfig({
  test: {
    coverage: {
      enabled: true,
      provider: new CoverageModule(),
    },
  },
});
❯ npm test
$ vitest
called

I'm not sure when exactly this has been fixed - by accident. I've had some tests for custom reporters in separate feature branch and earlier those were not working at all. Now they seem to run nicely. I'll set up PR for those tests later.

Here's some draft for reference:
custom-coverage-provider.ts
vitest.config.ts

$ pnpm test

CustomProvider::clean

 RUN  v0.28.4 /Users/x/y/vitest/test/coverage-test
      Coverage enabled with custom-coverage-provider

CustomProvider::onBeforeFilesRun
CustomProvider::onFileTransform
CustomProvider::onFileTransform
...

 Test Files  3 passed (3)
      Tests  7 passed (7)
   Start at  12:03:47
   Duration  410ms (transform 159ms, setup 15ms, collect 133ms, tests 20ms)

 % Coverage report from custom-coverage-provider
CustomProvider::reportCoverage

It seems to work because the enabled is hardcoded in config?

Thus it does not trigger the buggy path in deepMerge util.

The config has a default value of { enabled: true, provider: 'c8' } when --coverage is written in cli options

@AriPerkkio
Copy link
Member

Well, there still seems to be a problem when custom provider is used in worker. takeCoverage here will be undefined as options.provider cannot be passed in WorkerData since it's not serializable.

export async function takeCoverageInsideWorker(options: CoverageOptions) {
if (options.enabled && options.provider) {
const { takeCoverage } = await resolveCoverageProvider(options.provider)
return await takeCoverage?.()
}
}

This option should probably not be the module instance at first place.

Is there any other option than to change the API for custom providers to be a path where the provider can be loaded from? Then it could be loaded and initialized inside worker.

AriPerkkio added a commit to AriPerkkio/vitest that referenced this issue Feb 5, 2023
@mmis1000
Copy link
Author

mmis1000 commented Feb 5, 2023

Well, there still seems to be a problem when custom provider is used in worker. takeCoverage here will be undefined as options.provider cannot be passed in WorkerData since it's not serializable.

export async function takeCoverageInsideWorker(options: CoverageOptions) {
if (options.enabled && options.provider) {
const { takeCoverage } = await resolveCoverageProvider(options.provider)
return await takeCoverage?.()
}
}

This option should probably not be the module instance at first place.

Is there any other option than to change the API for custom providers to be a path where the provider can be loaded from? Then it could be loaded and initialized inside worker.

I am not sure if the use path / module name option even work with typescript file. Does vite-node ever support random string user provided in import(identifier)?

Another idea would be use the vite.config.ts as input source of worker. And just load the config again there. Then you can get a copy of whatever function user import in main compiler. But It will break any provider that relies on a singleton instance. (Guess it should be fine because the c8-provider and istabul-provider also have multi instance.)

@sheremet-va
Copy link
Member

Is there any other option than to change the API for custom providers to be a path where the provider can be loaded from? Then it could be loaded and initialized inside worker.

If you need to use it inside a worker, it should be a path to the module that will be imported inside. Beware that it will always be two different instances.

@AriPerkkio
Copy link
Member

AriPerkkio commented Feb 6, 2023

Inside workers we can safely create/import instances of CoverageProviderModules. The takeCoverageInsideWorker is explicitly created for this.

I am not sure if the use path / module name option even work with typescript file. Does vite-node ever support random string user provided in import(identifier)?

Currently Vitest supports regular test reporters to be written in Typescript, loaded here:

let customReporterModule: { default: new () => C }
try {
customReporterModule = await runner.executeId(path)
}

Now that CoverageProviderModule needs to be imported inside worker while running in vite-node, I'm not sure how to do this.
@sheremet-va any pointers how to import Typescript files in src/runtime/entry.ts?

Edit: Looks like we can use testRunner.importFile('/absolute/path/to/custom-provider.ts', 'setup') inside worker. I have the loading setup working now. I'll refactor and setup PR with the fix later.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2023
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants