Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Inaccurate code coverage metrics #28

Open
amclin opened this issue Oct 15, 2019 · 5 comments
Open

Inaccurate code coverage metrics #28

amclin opened this issue Oct 15, 2019 · 5 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@amclin
Copy link
Owner

amclin commented Oct 15, 2019

Test coverage numbers are incorrectly significantly lower than they should be because Jest cannot track coverage for spawned processes. The integration test runs the full installer as a spawned process, so we are effectively testing a lot more lines than code coverage would lead us to believe.

This is a limitation in Jest:
jestjs/jest#5274

Once workaround is to use NYC to capture code coverage (since it can track spawned processes) and then wrap that with Jest:
jestjs/jest#3190 (comment)

@amclin amclin added bug Something isn't working help wanted Extra attention is needed labels Oct 15, 2019
amclin added a commit that referenced this issue Oct 15, 2019
install.js doesn't have accurate test coverage due to Jest
limitations. See #28
amclin added a commit that referenced this issue Oct 16, 2019
install.js doesn't have accurate test coverage due to Jest
limitations. See #28
@jamesliu-usb
Copy link

Getting a 403 when I attempt to push a fix/codeCoverage branch 😢

@amclin
Copy link
Owner Author

amclin commented Nov 20, 2019

NYC maintainers discourage using spawn-wrap:
jestjs/jest#5274 (comment)

@jamesliu-usb
Copy link

jamesliu-usb commented Nov 20, 2019

Wait for nyc 15 then? Or go with the custom spawning approach that node-tmp took w/ their testing strategy?

The way I read their warning made me think most of the concern was around edge cases run on Windows runtimes, which made the spawn-wrap approach seem like an okay-ish interim solution til nyc 15 is released.

Though I suppose if CI is run on Windows-based servers that could monkey-wrench things... but don't think it is? Worth confirming?

@amclin
Copy link
Owner Author

amclin commented Nov 20, 2019

Something is better than nothing. I'm happy caveating the edge case limitation with a //TODO to replace in the future when NYC 15 is available. NYC 15 is already in beta. Or we can look at using the beta version. It would only be needed in the generator, not the generated projects, so I think using a beta version could be acceptable as long as it doesn't have show-stopping bugs in our otherwise simple use case.

@amclin
Copy link
Owner Author

amclin commented Jan 8, 2020

This new(ish) small runner might be a good alternative to NYC:
https://www.npmjs.com/package/coverage-node

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants