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

added filter for statement statistics that contain null entries #40

Closed
wants to merge 1 commit into from

Conversation

SamNelson
Copy link

This should fix issue #39

JaKXz
JaKXz previously approved these changes Apr 29, 2017
Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

This is a fantastic contribution! Thank you :) and would love to see more in the future.

statementMeta = fileCoverage.statementMap;
Object.keys(statementStats).forEach(function (stName) {
var statementStats = Object.keys(fileCoverage.s).filter(function (stName) {
return fileCoverage.s[stName] !== null;
Copy link
Member

Choose a reason for hiding this comment

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

would it be overly poor form to use != null to capture any ones that are undefined as well? or would it be overkill to do && !== undefined?

var statementStats = Object.keys(fileCoverage.s).filter(function (stName) {
return fileCoverage.s[stName] !== null;
}),
statementMeta = fileCoverage.statementMap;
Copy link
Member

@JaKXz JaKXz Apr 29, 2017

Choose a reason for hiding this comment

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

I think this should be a var statementMeta line instead of the comma-list-declaration syntax, it's kind of hard to grok imho.

@bcoe
Copy link
Member

bcoe commented Apr 29, 2017

@JaKXz I'm pretty sure that in normal circumstances, getting a null statement should be impossible;

@SamNelson are you able to share the codebase that you were bumping into issues on? I think if we switch the order of istanbul and rewire things might start working for you -- and we won't need this fix.

@JaKXz
Copy link
Member

JaKXz commented Apr 29, 2017

@bcoe wouldn't it be good to have this passing anyways? in case there are other scenarios where there are null entries in statement statistics?

@bcoe
Copy link
Member

bcoe commented Apr 29, 2017

@JaKXz the problem is it would result in misreporting test coverage information -- except in bizarre situations, like the interaction we're seeing between babel-plugin-rewire and babel-plugin-istanbul, it should be impossible to have a null in a coverage map. If we'd been hiding this edge-case, we never would have found the bad interaction between these two libraries that was dropping coverage information on the ground.

@JaKXz JaKXz dismissed their stale review April 30, 2017 23:44

Based on @bcoe's point, @SamNelson I think you should just re-order the plugins. I believe @hzoo started a thread about figuring out how to better approach the babel plugin order problem, but I don't know where that is.

@bcoe
Copy link
Member

bcoe commented May 8, 2017

@SamNelson please feel free to reopen this discussion if you're still bumping into issues 👍 we've rolled out a few changes to nyc/istanbul-lib-instrument that I think should solve your problem.

@bcoe bcoe closed this May 8, 2017
arnabsen pushed a commit to arnabsen/istanbuljs that referenced this pull request Feb 20, 2024
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.

html reporter does not render source files
3 participants