-
-
Notifications
You must be signed in to change notification settings - Fork 360
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: add --exclude-after-remap option for users who pre-instrument their codebase #697
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.
Looks cool 👍
@@ -320,9 +331,18 @@ You can specify custom high and low watermarks in nyc's configuration: | |||
} | |||
``` | |||
|
|||
## Other advanced features | |||
## Parsing Hints (Ignoring Lines) |
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.
This whole section looks good to me 👍
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.
It seems like this will help me; I'll let you know once I've tried it out (but please don't wait on me to merge/release it)
@@ -96,7 +96,7 @@ | |||
"resolve-from": "^2.0.0", | |||
"rimraf": "^2.5.4", | |||
"signal-exit": "^3.0.1", | |||
"spawn-wrap": "^1.4.0", | |||
"spawn-wrap": "1.3.8", |
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've noticed some weirdness around pinning versions in npm 5; using =1.3.8
seems to avoid it
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.
Is there a reason spawn-wrap
was downgraded? There is a fix in version 1.4.0
that can be useful for people that want to run nyc in a container.
The issue in question: istanbuljs/spawn-wrap#49
I can confirm this fixed issues I was seeing with folders being specified for instrumentation/instrumentering |
The Problem Being Addressed
If someone is running tests against files that have been pre-instrumented in a codebase with corresponding source-maps (rather than using a JIT transpiler like babel-register) the a source-map will sometimes result in a source-files path being remapped to a folder that is covered by an exclude rule.
A user can now disable this behavior by setting
--exclude-after-remap
tofalse
, which will stop exclude logic from being applied after the application ofsource-maps
.In This Pull Request
--exclude-after-remap
caveat.spawn-wrap
because it was creatingundefined
folders while I attempted to test things; I've filed an issue.@thegecko I think there's a good chance that the behavior around source-map remapping was the root cause of your issues. The more I think about #696, I don't think there's any reason why we should need to use the relative path rather than the absolute path to the file ... there's a chance however this might have stepped around the issues described in this pull request.
@ljharb perhaps you'd like to take a quick look at this pull too -- I forget, I think you might have been bumping into something related to this?