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

Feature Request: Ignore insignificant lines when coalesce ranges #514

Closed
coreyfarrell opened this issue Dec 23, 2019 · 7 comments · Fixed by #525
Closed

Feature Request: Ignore insignificant lines when coalesce ranges #514

coreyfarrell opened this issue Dec 23, 2019 · 7 comments · Fixed by #525

Comments

@coreyfarrell
Copy link
Member

Take the following example code:

if (process.env.NOTSET) {
	console.log('not covered 1');
	console.log('not covered 2');
	console.log('not covered 3');
	/* nothing to cover on this line */
	console.log('also not covered');
}

With the release of #511 instead of reporting 2,3,4,6 as uncovered it reports 2-4,6 as uncovered. This is great, a further improvement would be if we could report 2-6 as uncovered since line 5 is insignificant. I haven't investigated what it would take to accomplish this but wanted to create a ticket. This would be especially nice in situations where a larger function is never called and contains lines which do not have any coverage counters.

CC @piranna

@coreyfarrell coreyfarrell changed the title Feature Request: Ignore insigificant lines when coalesce ranges Feature Request: Ignore insignificant lines when coalesce ranges Dec 23, 2019
@piranna
Copy link
Contributor

piranna commented Dec 23, 2019

Good sugestion :-) It could make sense both ways, both to include insignificant lines like comments or whitelines since in fact they are not covered, or not include them because they're not code, but I think like you that it would not hurt :-)

I think it would be easy to implement if we have the line numbers and/or ranges of insignificant lines, and "extend" the current ranges to include them. Question here would be if they should be added only the insignificat lines between two uncovered lines, or also the "outer" insignificant lines between a covered and uncovered lines. I think the first option would be better one, what do you think?

@coreyfarrell
Copy link
Member Author

I think only inner insignificant lines should be used to extend the ranges. So:

if (false) {
  /* line 2 */
  console.log('line 3');
  console.log('line 4');
  /* line 5 */
  console.log('line 6');
}

IMO the best output would be to report lines 3-6 are not covered, line 2 should not be included as doing so does not help reduce the output. This is a simple example, the more practical example is a function that is never called and contains a dozen branches with a blank line after each. The fact that those blank lines in between are not "missed" is unnecessary information, I'd rather see one large range instead of a dozen small ranges.

CC @bcoe sorry I should have tagged you in the first place. Not sure this makes as big a difference for c8 since c8 doesn't ignore insignificant lines?

@piranna
Copy link
Contributor

piranna commented Dec 23, 2019

I think only inner insignificant lines should be used to extend the ranges.

I think so, too :-)

@piranna
Copy link
Contributor

piranna commented Jan 18, 2020

I have seen lately this problem myself. How can we move it forward? Where could I start looking? Does reports have the info of both covered, uncovered, useful and useless lines?

@coreyfarrell
Copy link
Member Author

@piranna sorry I've been slow to respond, very busy lately with $dayjob work.

The raw information is available to the nodeMissing function using fileCoverage.getLineCoverage(). This function will return an object, for example the following object:

{
  1: 1
  2: 0
  4: 0
}

This object says that line 1 was executed one time, line 2 and 4 were not executed and line 3 is insignificant. An example source that I believe would produce this result:

if ('NOT_SET' in process.env) {
  console.error('Unexpected environment `NOT_SET` was set');
  // Fatal error
  process.exit(1);
}

@piranna
Copy link
Contributor

piranna commented Feb 3, 2020

So, the way to get this, would be to run over the result of fileCoverage.getLineCoverage() creating ranges by joining the consecutive not executed lines, is that correct?

@piranna
Copy link
Contributor

piranna commented Feb 3, 2020

I've already implement this in pull-request #525.

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 a pull request may close this issue.

2 participants