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: add --exclude-after-remap option for users who pre-instrument their codebase #697

Merged
merged 2 commits into from
Oct 23, 2017

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Oct 23, 2017

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 to false, which will stop exclude logic from being applied after the application of source-maps.

In This Pull Request

  • added/updated docs:
    • adding a note about the --exclude-after-remap caveat.
    • documenting istanbul ignore rules (CC: @kentcdodds, I would love your 👀 on this since you just added a new exclude rule).
  • added a section to the test-suite specifically for source-maps (which are a common source of headaches for people, let's try to get more tests in place as people bump into issues).
  • I also downgraded spawn-wrap because it was creating undefined 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?

Copy link
Member

@kentcdodds kentcdodds left a 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)
Copy link
Member

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 👍

Copy link
Contributor

@ljharb ljharb left a 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",
Copy link
Contributor

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

Copy link

@rumpl rumpl Oct 30, 2017

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

@bcoe bcoe merged commit cdfdff3 into master Oct 23, 2017
@bcoe bcoe deleted the sourcemap-tests branch October 23, 2017 04:31
This was referenced Oct 29, 2017
@thegecko
Copy link
Contributor

I can confirm this fixed issues I was seeing with folders being specified for instrumentation/instrumentering

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.

5 participants