Include all source files in the report #275

gustavnikolaj · 2014-11-19T10:03:30Z

I wanted to have all my source files included in the report, no matter if they were tested or not, to provide a truthful picture of the actual coverage in the test suite, without doing weird hacks.

I tried using the --preload-sources option, but that had the unwanted side effect that it made it seem like you had quite a bit of extra coverage, which is the result of actually requireing the code.

So instead I tried implementing it by instrumenting the files which have not already been visited after the report has been built, and add those. The only workaround I needed to make was to reset the count of visits on FunctionDeclarations as those were given a count of one when instrumenting, to compensate for function hoisting.

The feature is hidden behind the --include-all-sources flag.

I feel that this pull request covers the case of #142, in a simpler way than what you outlined in #142 (comment). Am I missing something?

gotwarlost · 2014-12-03T22:21:40Z

Nicely done.

There is no need to create new instances of the instrumenter and transformer, is there? Please re-use existing objects so that everything is initialized in one place.
Please add a test (or hijack any test for preload sources to test this; see below)
I think it is weird to have both --preload-sources and --include-all-sources for the same command. Since your implementation is what people expect, let's do the following:
- support both options and maintain backwards compatibility of interface
- if --preload-sources is true just make include-all-sources true. Print a message to stderr saying that the preload option is deprecated and to use the new option instead.
Ensure that istanbul configuration is loaded correctly and there is a way to set your option in the config file (I think you have already done this, but it's been a while since I've looked at the config code so not sure)

hidden behind the --include-all-sources flag After the coverage report has been built, manually instrument the files which have not been visited already, and add those to the report. When instrumenting the code manually, reset the count in the coverState object. FunctionDeclaration is given a visit number of 1 when instrumenting to compensate for function hoisting. We don't want that in this case, as the file was not actually loaded.

gustavnikolaj · 2014-12-04T12:22:45Z

Thanks for the very thorough feedback! Totally agree with everything - I was a bit lazy when first sending this pull request, as I wanted to verify the solution before putting in too much work. :-)

I rebased the pull request onto current master.
I looked into the code, and there is no reason why I cannot reuse the instrumenter and transformer. The state in the instrumenter is reset every time transformer (or rather instrumentAstSync) is called. So I removed that.
I wrote a test for the feature based on the preload sources test. Which also ensures that the flag works from the command line.
I added a test to ensure that the default value is false and that it is set per default. I did not find any excisting test loading configuration files
I removed everything related to the preload sources feature, except some basic configuration stuff, to be able to warn when I get the flag.

I'm not sure about the way that I chose to warn the users when using the preload-sources switch - there might be a better place to do it. But it was the best place I could find / think of.

gotwarlost · 2014-12-04T17:02:09Z

lib/config.js

@@ -24,6 +24,7 @@ function defaultConfig() {
            'complete-copy': false,
            'save-baseline': false,
            'baseline-file': './coverage/coverage-baseline.json',
+            'include-all-sources': false,
            'preload-sources': false


Can we remove the preload-sources key from the default config safely? (that object is printed as help for the default configuration: istanbul help config). I don't believe this will impact anything but this needs to be verified.

gotwarlost · 2014-12-04T17:03:07Z

Excellent work! Could you attempt to remove the preload-sources option from the config (see my line comment). Just make sure nothing breaks because of this.

gustavnikolaj · 2014-12-04T20:15:44Z

Removing preload-sources from the default config means that it wont be set in the object. I suspect that the reason is that no config option should not have a default value, which makes kind of sense. If this is unintended and not considered a feature, we could change that too, but I'd say that it's outside the scope of this pull request.

So it's still present in the parsing of options passed to run-with-cover.js, to be propagated through to the configuration loader. And it's still present in the configuration loader, as it has to be there to be allowed to have effect.

We might be able to golf down the number of "preload-sources" occurances in the code by moving the warning logic into run-with-cover.js - but it seems to be more proper to have that check in the configuration loading code, as that would catch all occurances of that flag - and not just those passed on the commandline to the istanbul cover method. I'm not sure if there's any other way of doing it, as that is how I normally use the tool - but I assume that you could pass it in via a yaml config file aswell? If I'm right about that, moving it out of the configuration code would mean that the warning would go unnoticed for people using config files, and that's not optimal of course.

I'll be more than willing to change it, if any of my conclusions are wrong. :-)

gotwarlost · 2014-12-04T20:33:57Z

I'm going to merge this and deal with that specific issue later. Thanks for all the hard work!

…dedFilesInReport Include all source files in the report

gustavnikolaj · 2014-12-04T23:05:35Z

No problem! It's a pleasure to be able to contribute a little back to a project that you use a lot :-)

gotwarlost · 2014-12-07T17:30:56Z

Available in v0.3.5

gustavnikolaj · 2014-12-07T17:32:09Z

Yay! Thanks :-)

slonoed · 2015-04-10T15:13:29Z

Hi all!

I need some help with this. I use babel and es6 syntax. I pass --compilers js:babel/register to mocha, but other files (required by istanbul) not parsed.

ghost · 2015-04-10T16:10:33Z

Is there an example somewhere of how to use this flag? I've tried

istanbul cover --include-all-sources jasmine
and
istanbul cover --include-all-sources --include-pattern=**/*.js jasmine

but I'm not seeing any difference in output… I'm using v0.3.13.

Cheers!

gustav-olsen-groupone added 5 commits December 4, 2014 07:30

add test for includeAllSources

79ad6e6

do not reinstantiate transformer and instrumenter

f17d1db

assert that the default value for includeAllSources is set as expected

8bdf170

remove preload-sources option and add deprecation warning.

c5cb52d

gotwarlost reviewed Dec 4, 2014
View reviewed changes

gotwarlost added a commit that referenced this pull request Dec 4, 2014

Merge pull request #275 from gustavnikolaj/feature/includeAllNonExclu…

06ff06d

…dedFilesInReport Include all source files in the report

gotwarlost merged commit 06ff06d into gotwarlost:master Dec 4, 2014

gustavnikolaj deleted the feature/includeAllNonExcludedFilesInReport branch December 4, 2014 23:04

iknowcss mentioned this pull request Dec 31, 2014

Giving results for files that does not have corresponding test taichi/grunt-istanbul#14

Open

tannerlyons mentioned this pull request Jan 13, 2015

Allow including all source files in coverage reports karma-runner/karma-coverage#125

Closed

ngrieble mentioned this pull request Feb 16, 2015

Get Code Coverage for non-tested files asciidisco/grunt-qunit-istanbul#40

Open

springmeyer mentioned this pull request Jul 8, 2015

tilelive.copy is undocumented mapbox/tilelive#138

Closed

Aaronius mentioned this pull request Oct 9, 2015

Add option for including all sources webpack-contrib/istanbul-instrumenter-loader#15

Open

westonpace mentioned this pull request Aug 17, 2016

Option for Istanbul coverage to include all source files (as opposed to only files loaded during test) angular/angular-cli#1735

Open

mattdell mentioned this pull request Mar 7, 2017

Documentation for --include-all-sources #767

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include all source files in the report #275

Include all source files in the report #275

gustavnikolaj commented Nov 19, 2014

gotwarlost commented Dec 3, 2014

gustavnikolaj commented Dec 4, 2014

gotwarlost Dec 4, 2014

gotwarlost commented Dec 4, 2014

gustavnikolaj commented Dec 4, 2014

gotwarlost commented Dec 4, 2014

gustavnikolaj commented Dec 4, 2014

gotwarlost commented Dec 7, 2014

gustavnikolaj commented Dec 7, 2014

slonoed commented Apr 10, 2015

ghost commented Apr 10, 2015

Include all source files in the report #275

Include all source files in the report #275

Conversation

gustavnikolaj commented Nov 19, 2014

gotwarlost commented Dec 3, 2014

gustavnikolaj commented Dec 4, 2014

gotwarlost Dec 4, 2014

Choose a reason for hiding this comment

gotwarlost commented Dec 4, 2014

gustavnikolaj commented Dec 4, 2014

gotwarlost commented Dec 4, 2014

gustavnikolaj commented Dec 4, 2014

gotwarlost commented Dec 7, 2014

gustavnikolaj commented Dec 7, 2014

slonoed commented Apr 10, 2015

ghost commented Apr 10, 2015