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: Fix excludeAfterRemap functionality. #1010

Merged
merged 3 commits into from
Mar 7, 2019

Conversation

coreyfarrell
Copy link
Member

I'm not sure if this qualifies as a feature or a fix, filtering after remapping has never worked. Original implementation of filtering remapped sources at dd40dc5, addition of excludeAfterRemap option at cdfdff3. Unfortunately the filtering step was done before coverage was remapped using source-maps, so it didn't do anything.

This is a potentially breaking change - bundled / compiled code was never filtered based on the original source filename, after this it will be. As an auxiliary change test-exclude options are also added to the nyc report and nyc check-coverage as those commands should process exclude-after-remap.

Fixes #956

@coveralls
Copy link

coveralls commented Mar 6, 2019

Coverage Status

Coverage decreased (-0.3%) to 93.701% when pulling 220f77d on coreyfarrell:exclude-after-remap into e501d86 on istanbuljs:master.

@bcoe
Copy link
Member

bcoe commented Mar 6, 2019

@coreyfarrell see my comments here; do we just bite the bullet and move away from the --exclude-after-remap command; perhaps it would give us more flexibility for a variety of configurations if we broke this out into its own configuration setting?

Previously excludeAfterRemap caused filtering to happen before remapping
to original source filenames.  This caused the filtering to be
completely ineffective.  Fix the order, add a test to verify
functionality.
@coreyfarrell coreyfarrell merged commit 0fc217e into istanbuljs:master Mar 7, 2019
@coreyfarrell coreyfarrell deleted the exclude-after-remap branch March 7, 2019 22:05
})
.option('include', {
alias: 'n',
default: [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coreyfarrell sorry for the post-review - shouldn't this be ['**']?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. An empty list is functionally equal to ['**'] but [] actually bypasses the check so it's more efficient.

https://github.com/istanbuljs/istanbuljs/blob/master/packages/test-exclude/index.js#L50-L54 is where the default is processed, [] is replaced with false which short-circuits the include check.

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.

How to ignore coverage data (instrumented code) from dependencies?
4 participants