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

Missing function coverage when transform prepends code right before the function #7130

Open
6 tasks done
hi-ogawa opened this issue Dec 25, 2024 · 4 comments · May be fixed by #7192
Open
6 tasks done

Missing function coverage when transform prepends code right before the function #7130

hi-ogawa opened this issue Dec 25, 2024 · 4 comments · May be fixed by #7192
Labels
feat: coverage Issues and PRs related to the coverage feature p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Dec 25, 2024

Describe the bug

EDIT: I made a simpler reproduction with user land transform #7130 (comment).

related:

After vitejs/vite#19004, a following code is transformed in this way (source map vis)

input

export function f(a: number, b: number) {
	return a + b;
}

import "node:path"

output

const __vite_ssr_import_0__ = await __vite_ssr_import__("node:path");function f(a, b) {
  return a + b;
}
Object.defineProperty(__vite_ssr_exports__, "f", { enumerable: true, configurable: true, get(){ return f }});;

This seems to cause function f to be considered not covered even when f is executed. I confirmed Vite commit right before vitejs/vite#19004 didn't have this issue.


Independently from vitejs/vite#19004, I'm trying to hoist Object.defineProperty in vitejs/vite#18983 and that seems to cause a similar missing coverage.

Transform works like this (source map vis)

input

export function f(a: number, b: number) {
	return a + b;
}

output

Object.defineProperty(__vite_ssr_exports__, "f", { enumerable: true, configurable: true, get(){ return f }});function f(a, b) {
  return a + b;
}

People usually don't write import after the export like the first case and we didn't have a test case for that, so I didn't notice it when reviewing vitejs/vite#19004. While testing my PR #7096, this just got caught because it broke the coverage test, which has this pattern (export function at the first line):

export function sum(a: number, b: number) {

Reproduction

https://github.com/hi-ogawa/reproductions/tree/main/vitest-function-coverage-vite-19004
https://github.com/hi-ogawa/reproductions/tree/main/vitest-function-coverage-prepend-transform

System Info

System:
    OS: Linux 6.12 Arch Linux
    CPU: (16) x64 12th Gen Intel(R) Core(TM) i7-12650H
    Memory: 10.09 GB / 31.05 GB
    Container: Yes
    Shell: 5.2.37 - /usr/bin/bash
  Binaries:
    Node: 20.18.1 - ~/.volta/tools/image/node/20.18.1/bin/node
    npm: 10.8.2 - ~/.volta/tools/image/node/20.18.1/bin/npm
    pnpm: 9.15.1 - ~/.volta/bin/pnpm
    bun: 1.1.29 - ~/.volta/bin/bun
  Browsers:
    Chromium: 131.0.6778.85
  npmPackages:
    @vitest/coverage-v8: 3.0.0-beta.3 => 3.0.0-beta.3 
    vitest: 3.0.0-beta.3 => 3.0.0-beta.3

Used Package Manager

pnpm

Validations

@hi-ogawa hi-ogawa added the feat: coverage Issues and PRs related to the coverage feature label Dec 25, 2024
@hi-ogawa
Copy link
Contributor Author

The 2nd one is because the lines with __vite_ssr_exports__ is stripped away for v8 coverage.

const VITE_EXPORTS_LINE_PATTERN
= /Object\.defineProperty\(__vite_ssr_exports__.*\n/g

trimmed.replaceAll(VITE_EXPORTS_LINE_PATTERN, '\n')

@hi-ogawa hi-ogawa added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Jan 8, 2025
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 8, 2025

I think this is a bug in coverage conversion and probably we can fix it. I made a simpler reproduction with user land transform plugin. https://github.com/hi-ogawa/reproductions/tree/main/vitest-7130-prepended-function-coverage

Probably the issue is that istanbul fails to filter out "injected function" from the coverage when it lives in the same line which has mappings. https://github.com/istanbuljs/v8-to-istanbul/blob/46b972458b40ea0d628c80dd60223cc909f81cd0/lib/v8-to-istanbul.js#L144-L149

For example, for the following generated code, istanbul shows prepended in fnMap while appended is removed.

// source
console.log("x");

// generated
function prepended(){};console.log("x");
function appended(){};

Hmm, actually the reason why appended is filtered properly is because Vitest got this patch?

@hi-ogawa hi-ogawa linked a pull request Jan 8, 2025 that will close this issue
6 tasks
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 9, 2025

The issue might be actually same for istanbul provider based on the same simple reproduction of #7130 (comment).
I thought I only saw issues with v8 for Vite SSR transform, but I need to look into that again.


Probably relevant coverage filtering logic on istanbul is here https://github.com/istanbuljs/istanbuljs/blob/06eec782dc8a248f0516cdba06b280c410515890/packages/istanbul-lib-source-maps/lib/transformer.js#L34-L62.

@hi-ogawa hi-ogawa changed the title Missing function coverage (v8) when transform prepends code right before the function Missing function coverage when transform prepends code right before the function Jan 9, 2025
@AriPerkkio
Copy link
Member

Hmm, actually the reason why appended is filtered properly is because Vitest got this patch?

istanbuljs/v8-to-istanbul#244

This patch only affects line coverage of source files. Lines of transformed files do not affect that patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: coverage Issues and PRs related to the coverage feature p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants