-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use a custom esm cache for coverage testing. #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already chatted about this quite a bit offline. LGTM.
test/package.json
Outdated
@@ -4,8 +4,8 @@ | |||
"private": true, | |||
"scripts": { | |||
"test": "node --preserve-symlinks test.js test", | |||
"coverage": "cross-env NODE_ENV=test nyc --reporter=lcov --reporter=text-summary npm test", | |||
"coverage-ci": "cross-env NODE_ENV=test nyc --reporter=lcov npm test", | |||
"coverage": "cross-env NODE_ENV=test ESM_OPTIONS='{cache:\"node_modules/.cache/esm-coverage\"}' nyc --reporter=lcov --reporter=text-summary npm test", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, can this be part of the package.json or other .npmFOO thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to make this more DRY, but I wasn't sure how that didn't add complexity. We only want ESM_OPTIONS set for coverage commands, so any sort of global config doesn't seem like it would work. I had tried a cross-env-shell version that had a single common command and passed args to it, but that seemed more complex. I pushed a patch to change to disable the cache. Perhaps in the future esm can be fixed or ESM_DISABLE_CACHE=true will work for this use case.
For anyone testing, you may need to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and it works.
Codecov Report
@@ Coverage Diff @@
## main #6 +/- ##
=======================================
Coverage 93.33% 93.33%
=======================================
Files 4 4
Lines 75 75
=======================================
Hits 70 70
Misses 5 5 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also fine with this approach.
- Disabling the esm cache was a preferred fix rather than than more verbose setting of a cache directory. - This also may ease the transition to ESM_DISABLE_CACHE if that becomes more functional for this use case.
e0c8501
to
c08a686
Compare
This replaces #4 as a way to fix coverage testing.
c8
may still be an option, but this patch is to fix the underlyingesm
issue.This fix sets a custom
esm
cache directory when running coverage testing. Without a fix like this, mixed running of regular testing and coverage testing can result in incorrect coverage results.This pattern may end up applying to many other packages. Please test and suggest improvements.