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

feat: exclude lines that are missing from source maps #244

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AriPerkkio
Copy link
Contributor

@AriPerkkio AriPerkkio commented Mar 23, 2024

feat: exclude lines that are missing from source maps

  • When source maps are available, use @jridgewell/trace-mapping to figure out which lines contain runtime code. If a line is not present in source maps, consider it as empty line. This will exclude Typescript typings, comments, empty lines and any other special syntax that transpiled languages exclude from source maps.
  • Should this be considered as breaking change or be a configurable option? ⚠️
    • This is now opt-in and configurable via excludeEmptyLines argument. No breaking changes.

Examples:

  • Vitest

  • Jest

  • Playwright

  • c8, test code transpiled with tsc. (tsconfig.json's removeComments was disabled when taking the picture)

fix: ignore hint to exclude lines

  • /* v8 ignore ... */ should exclude the lines from coverage, not mark them as covered.

@AriPerkkio
Copy link
Contributor Author

AriPerkkio commented Mar 23, 2024

Any thoughts about this approach @SimenB and @bcoe?

In Vitest we will always have source maps available so we can utilize those for analysing the source file. I believe Jest has them too? I'm not that sure about c8 though.

When source maps are not passed, the empty lines should be marked as covered as before.

@joshuanathanson
Copy link

Possible to get this merged? We're currently forking this commit so we can get proper coverage numbers in our repo. Def not ideal.

@AriPerkkio
Copy link
Contributor Author

AriPerkkio commented May 15, 2024

@joshuanathanson - just curious, what test runner and coverage tool are you using this commit with?

@joshuanathanson
Copy link

@joshuanathanson - just curios, what test runner and coverage tool are you using this commit with?

We have our own wrappers, but under the hood we're using Selenium to run the tests, along with grabbing the coverage from Chrome on the fly with Selenium CDP DevTools.

@SimenB
Copy link
Member

SimenB commented May 15, 2024

This is one of those I'd love to hear @bcoe's thoughts 👀

@lo1tuma
Copy link

lo1tuma commented May 21, 2024

I think this would also fix the related issue in c8. Would be really nice to get this merged.

@joshuanathanson
Copy link

Bump on this? Revisiting our work in this area and would be nice to get this merged.

@AriPerkkio
Copy link
Contributor Author

This feature has been included in Vitest v1.6 and enabled by default in Vitest v2 for ~2 weeks now. We haven't (yet) heard any issues about this change.

@bcoe there are many issues in c8 that would be fixed by this PR (as long as source maps are available there):

@ericmorand
Copy link

Isn't there a risk that lines removed by treeshaking are marked as empty? This is maybe not an issue (after all if they are not in the built artifact, it means that they are not relevant) but it may look strange in reports that they are marked as empty.

@AriPerkkio
Copy link
Contributor Author

@ericmorand yup, those lines cannot be covered so I would expect them to be excluded from report. This is similar how nyc and Jest (with default babel provider) works.

@AriPerkkio
Copy link
Contributor Author

Looks like node:test started to exclude lines that are not present in source maps too: https://github.com/nodejs/node/pull/55228/files#diff-f93d30118ca2d349a10c8f93d740e1c988671a2e35ffe1213fedd6868df60e61R48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comments are reported as not covered
5 participants