-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Incorrect Jest coverage #3854
Comments
I added this to my Jest configuration in "jest": {
"collectCoverage": true,
"transform": {
"^.+\\.(t|j)sx?$": [
"@swc/jest",
{
"sourceMaps": true
}
]
}
} |
@pspeter3 Thank you for your code snippet, it helps, but now there is another problem: It looks like not covered code highlights are not displayed correctly. There is an example of coverage report from project with:
the yellow highlight here says that the comma is supposedly not covered |
* upgrade to next v12 * removed babel for swc * npm updates including msw and lint * azure pipeline now builds with node v16 * swc/jest is not reporting coverage correctly https://github.com/swc-project/jest/issues/21
@krutoo ran into the same issues and guessed it is caused by the conversion to older JS syntax. We could fix these false-positives by targeting a newer ES version.
|
@sebald Thank you, looks like it realy works with |
Another reproduction here: typescript-eslint/tslint-to-eslint-config#1367 I ended up only needing |
I think swc reusing span for various places may cause this. Can you try https://sokra.github.io/source-map-visualization/#custom by manually invoking swc? |
@kdy1 does this help? Maybe the Edit: tested with
|
@bobaaaaa Can you share some code? I think the test file would be enough, as it does not have good sourcemap. |
Oh... Maybe assumption about monotonic increment of source map position can be the cause. |
@kdy1 You can find it here: https://gist.github.com/bobaaaaa/3649b3a7e6312793a257bf67c500128a (thx for investigating ❤️) |
You need |
@kdy1 Hm, I tested both |
Hmm... Source maps are valid, maybe emitted tokens without sourcemap entry can be the cause I guess? |
@kdy1 I updated the gist with the generated .js + .js.map files: https://gist.github.com/bobaaaaa/3649b3a7e6312793a257bf67c500128a |
For my NestJS project, I also noticed that test coverage dropped quite a bit when switching from Digging into it, it seems to be related to how the Typescript metadata for constructors are transpiled. Take this constructor:
The output of
Notice the Compare this to how
No ternary, so no impact to code coverage. I created a workaround that tells istanbul to ignore these lines that incorrectly impact code coverage. It inserts an
I setup this repo with instructions to reproduce: https://github.com/rogisolorzano/nest-swc-coverage The workaround I'm using is in create-swc-transformer.js Curious if there are any thoughts on this and potential solutions that don't involve having to insert |
I saw mention of using I'm using NestJS as well and this seems to be related to decorators, so removing the This seems to be very related to an issue I think I've tried all configuration permutations listed here so far without any luck, as well as installing Potentially Related Issues on Other Projects
Ultimately it sounds like source mapping can help some scenarios, changing the target in other scenarios, but specific decorators don't seem to care about either of those in some scenarios like Angular and NestJS commonly it seems. |
I've tested here with vitest+swc in a Nestjs project and the issue still happens. I can't apply a transformer as @rogisolorzano suggested (I guess transformers are just for Jest). There's also an issue in istanbul related to the same problem: istanbuljs/istanbuljs#70 |
I took some time to checkout the related "fix for decorator coverage" that I linked above. It does seem they took the same approach as @rogisolorzano 👍 I'm not sure if this project has a way to implement a "pre-processor" but maybe that's something @rogisolorzano could open a PR for is this is how folks seem to be getting around this. Otherwise it sounds like there needs to be a major rework on the system to implement so that it doesn't require a ternary (or is this an underlying transpilation implementation issue of these frameworks with how the decorator is written?). PS: Thanks @rogisolorzano for the minimal reproduction and digging deep into this. @klutzer are you not using Jest? I believe that's the scope of this issue, but I believe the coverage issue is not directly related to jest but the coverage tool [istanbul]. |
This comment has been minimized.
This comment has been minimized.
i'm using vitest instead of jest, but i'm dealing with the same coverage issues outlined above in my nestjs project. i ended up creating a custom plugin that adds my /**
* @file Vitest Configuration
* @module config/vitest
* @see https://vitest.dev/config/
*/
import { DECORATOR_REGEX } from '@flex-development/decorator-regex'
import pathe from '@flex-development/pathe'
import { cast, split, type Nullable } from '@flex-development/tutils'
import swc from '@swc/core'
import ci from 'is-ci'
import tsconfigpaths from 'vite-tsconfig-paths'
import GithubActionsReporter from 'vitest-github-actions-reporter'
import {
defineConfig,
type UserConfig,
type UserConfigExport
} from 'vitest/config'
import { BaseSequencer, type WorkspaceSpec } from 'vitest/node'
import tsconfig from './tsconfig.json' assert { type: 'json' }
/**
* Vitest configuration export.
*
* @const {UserConfigExport} config
*/
const config: UserConfigExport = defineConfig((): UserConfig => {
/**
* [`lint-staged`][1] check.
*
* [1]: https://github.com/okonet/lint-staged
*
* @const {boolean} LINT_STAGED
*/
const LINT_STAGED: boolean = !!Number.parseInt(process.env.LINT_STAGED ?? '0')
return {
define: {},
plugins: [
{
enforce: 'pre',
name: 'decorators',
/**
* Transforms source `code` containing decorators.
*
* @see https://github.com/swc-project/swc/issues/3854
*
* @param {string} code - Source code
* @param {string} id - Module id of source code
* @return {Promise<Nullable<{ code: string }>>} Transform result
*/
async transform(
code: string,
id: string
): Promise<Nullable<{ code: string }>> {
// do nothing if source code does not contain decorators
DECORATOR_REGEX.lastIndex = 0
if (!DECORATOR_REGEX.test(code)) return null
/**
* Regular expression used to match constructor parameters.
*
* @see https://regex101.com/r/kTq0JK
*
* @const {RegExp} CONSTRUCTOR_PARAMS_REGEX
*/
const CONSTRUCTOR_PARAMS_REGEX: RegExp =
/(?<=constructor\(\s*)([^\n)].+?)(?=\n? *?\) ?{)/gs
// add "/* c8 ignore next */" before constructor parameters
for (const [match] of code.matchAll(CONSTRUCTOR_PARAMS_REGEX)) {
code = code.replace(match, (params: string): string => {
return split(params, '\n').reduce((acc, param) => {
return acc.replace(
param,
param.replace(/(\S)/, '/* c8 ignore next */ $1')
)
}, params)
})
}
return {
code: (
await swc.transform(code, {
configFile: false,
filename: id,
inlineSourcesContent: true,
jsc: {
keepClassNames: true,
parser: {
decorators: true,
dynamicImport: true,
syntax: 'typescript',
tsx: pathe.extname(id) === '.tsx'
},
target: cast<swc.JscTarget>(tsconfig.compilerOptions.target),
transform: {
decoratorMetadata:
tsconfig.compilerOptions.emitDecoratorMetadata,
legacyDecorator: true,
useDefineForClassFields:
tsconfig.compilerOptions.useDefineForClassFields
}
},
sourceMaps: 'inline',
swcrc: false
})
).code
}
}
},
tsconfigpaths({ projects: [pathe.resolve('tsconfig.json')] })
],
test: {
allowOnly: !ci,
benchmark: {},
chaiConfig: {
includeStack: true,
showDiff: true,
truncateThreshold: 0
},
clearMocks: true,
coverage: {
all: !LINT_STAGED,
clean: true,
cleanOnRerun: true,
exclude: [
'**/__mocks__/**',
'**/__tests__/**',
'**/index.ts',
'src/interfaces/',
'src/metadata/',
'src/types/'
],
extension: ['.ts'],
include: ['src'],
provider: 'v8',
reporter: [...(ci ? [] : (['html'] as const)), 'lcovonly', 'text'],
reportsDirectory: './coverage',
skipFull: false
},
environment: 'node',
environmentOptions: {},
globalSetup: [],
globals: true,
hookTimeout: 10 * 1000,
include: [
`**/__tests__/*.${LINT_STAGED ? '{spec,spec-d}' : 'spec'}.{ts,tsx}`
],
isolate: true,
mockReset: true,
outputFile: { json: './__tests__/report.json' },
passWithNoTests: true,
reporters: [
'json',
'verbose',
ci ? new GithubActionsReporter() : './__tests__/reporters/notifier.ts'
],
/**
* Stores snapshots next to `file`'s directory.
*
* @param {string} file - Path to test file
* @param {string} extension - Snapshot extension
* @return {string} Custom snapshot path
*/
resolveSnapshotPath(file: string, extension: string): string {
return pathe.resolve(
pathe.resolve(pathe.dirname(pathe.dirname(file)), '__snapshots__'),
pathe.basename(file).replace(/\.spec.tsx?/, '') + extension
)
},
restoreMocks: true,
root: process.cwd(),
sequence: {
sequencer: class Sequencer extends BaseSequencer {
/**
* Determines test file execution order.
*
* @public
* @override
* @async
*
* @param {WorkspaceSpec[]} specs - Workspace spec objects
* @return {Promise<WorkspaceSpec[]>} `files` sorted
*/
public override async sort(
specs: WorkspaceSpec[]
): Promise<WorkspaceSpec[]> {
return (await super.sort(specs)).sort(([, file1], [, file2]) => {
return file1.localeCompare(file2)
})
}
}
},
setupFiles: ['./__tests__/setup/index.ts'],
silent: false,
singleThread: true,
slowTestThreshold: 3000,
snapshotFormat: {
callToJSON: true,
min: false,
printBasicPrototype: false,
printFunctionName: true
},
testTimeout: 10 * 1000,
typecheck: {
allowJs: false,
checker: 'tsc',
ignoreSourceErrors: false,
include: ['**/__tests__/*.spec-d.ts'],
tsconfig: pathe.resolve('tsconfig.typecheck.json')
},
unstubEnvs: true,
unstubGlobals: true
}
}
})
export default config |
I think #7900 may improve the situation. |
Our issue was that components that were using emotion were being tested but not captured in the coverage reporting. Using |
- remove babel - usw swc transform - fix wallaby config - remove coverage thresholds due to swc-project/swc#3854 - generate coverage report as PR comment instead
- remove babel - usw swc transform - fix wallaby config - remove coverage thresholds due to swc-project/swc#3854 - generate coverage report as PR comment instead
- remove babel - usw swc transform - fix wallaby config - remove coverage thresholds due to swc-project/swc#3854 - generate coverage report as PR comment instead
@kdy1, would @rogisolorzano's suggestion work for a definitive solution in |
I'll give this another try tomorrow. Thanks! |
fwiw, i created a simple plugin to fix this in my projects, as @rogisolorzano did not work for me (probably because I am using v8 for coverage like @unicornware). it works for the specific problem @rogisolorzano described, but can't say it's a complete solution. also my first foray into Rust so constructive feedback is welcome. |
Okay, about the ternary case, |
Oh. I need to generate less source map. I fixed it for |
#9074 should fix this issue for users using |
**Description:** Coverage gets better if we generate fewer source map entries. This PR only fixes the issue for `"coverageProvider": "v8"`. Much more work is required for the default coverage provider I guess. **Related issue:** - #3854
the above fixed the coverage issue, but unfortunately with "coverageProvider": "v8" my tests crash half way through with heap out of memory error. also "v8" seems lot (maybe 2x) slower than with default coverage provider I was on "@swc/core": "1.6.13", |
The solution of @rogisolorzano solved it for us and was plug and play. The nice part is that their implementation doesnt forget to forward config to underlying module (useful if you customize it). Ex:
Our tools and configs: @swc/jest 0.2.36, @swc/core 1.6.13, jest 29.7.0, targetting commonjs. |
The issue still persists. I had to go back to
|
When using @swc/jest to transpile ts to commonjs and running
jest --coverage
certain branches are shown as not covered (console logging in these branches show that tests do run the code path). Using babel to transpile and run the tests shows the correct coverage.The text was updated successfully, but these errors were encountered: