add --all functionality

[bcoe]

we need to figure out a way for --all to work with this instrumentation approach, we might be able to work backwards from the --all functionality in nyc.

[bcoe]

see also: dtinth/babel-plugin-__coverage__#38

[bcoe]

@gotwarlost @dtinth, any thoughts as to how we might approach adding blank coverage for all files, without messing with the babel cache?

Perhaps we could just walk all the files, and add fake entries in __coverage__, could we add an API method for this to lib-instrument?

[danez]

Do you think this would be possible todo in the babel-plugin itself? To me it looks more like that nyc should traverse all files and add blank coverage before running the tests. Because on a test-suite without tests, the plugin wouldn't be called at all, but you might still want to get an report telling you 0% right?

I would like to give this a shoot over the weekend, so if you have hints were to start let me know please.

[bcoe]

@danez I'm not 100% sure of the best solution; folks might be using different approaches to instrumenting code with babel, etc., so my temptation was to try to do it in the plugin layer -- the other reason being that the plugin knows about include/exclude rules -- which we want to make sure we adhere to.

my temptation was to think about logic similar to nyc: https://github.com/istanbuljs/nyc/blob/master/index.js#L220 perhaps this is a good starting point?

I'm not an expert with babel, so I'm sure your approach might be better, really I'm open to experimentation.

[danez]

I had a look, but I couldn't figure out an easy way to do this and spent most of the time understanding how nyc works :)
In general it feels wrong to me to put this logic in the babel-plugin itself, as these would be side-effects that are not really nice. And even if the plugin would do it and instrument all included files, then it would still need to communicate the list of files to nyc somehow.

Maybe shipping an instrumenter adapter with this repo for nyc (like the noop.js) that handles and knows how to load stuff that was processed by this plugin.

I won't have time to look into to this again in the next weeks.

[lughino]

Hi, any news about this enhancement?

[benhunsaker]

has there been any update on this?

[wojtkowiak]

Any plans on working on this?

[motiz88]

My thoughts so far:

The correct representation for "uncovered file" is not some null object but rather a coverage object (with zero hit counts but also a list of statements, branches etc) that has not seen any line executed. IOW it is a byproduct of the instrumentation step. The correct way to get at that data, with the least amount of duplicated work, seems to be to parse the instrumented files and statically evaluate the initial coverage object.

[motiz88]

If anyone wants to save me some digging (@bcoe?) I'd appreciate a quick explanation of how the coverage variable gets named.

[bcoe]

@motiz88 I would definitely love to add the --all functionality to the plugin. We have an approach in nyc that works quite well:

https://github.com/istanbuljs/nyc/blob/master/index.js#L163

I think we might be able to use this as a starting point.

[bcoe]

@motiz88 the coverage variable is generated here for a given file:

https://github.com/istanbuljs/istanbul-lib-instrument/blob/master/src/visitor.js#L13

My gut is we can probably use something similar to the approach used in nyc to perform a walk with glob and generate empty reports.

[motiz88]

@bcoe That does seem to be the shell of a solution. The core then would be extracting this empty report from each already-instrumented file.

We could in theory re-parse the original file with a modified version of the instrumenting visitor; but the user's babel pipeline is largely outside our control (as nyc), and I worry that we could end up duplicating work, needing to know the user's exact build config, etc.

Therefore my approach is to try and statically extract the already-computed "empty coverage" object from each instrumented file. Looking at the code I think we would need to:

1. Parse the file using Babylon (with all parser plugins enabled - there could be any syntax in there)
2. Hash the filename using genVar() (currently in visitor.js);
3. Look for a variable in the global scope with that name, initialized to a function expression;
4. Within that function, get the expression initializing a variable called coverageData;
5. Statically evaluate that expression (which should be all literals).

If we made it through these 5 steps - we now have the equivalent of coverageData for the original file, as saved at the end of the instrumenting visitor's run.

[motiz88]

I'm going to work on this as a PR to istanbul-lib-instrument.

[motiz88]

@bcoe With istanbul-lib-instrument v1.2.0 released, will you also do a release of babel-plugin-istanbul depending on it? This would help kick old versions of the library from caches etc.

[bcoe]

@wojtkowiak @lughino @benhunsaker @danez, thanks to the hard work of @motiz88 there is now support for --all when using babel-plugin-istanbul and nyc. This is currently published to the next branch of nyc, and I would appreciate the extra set of eyes:

npm cache clear; npm i nyc@next

Your test script will end up looking something like this:

cross-env NODE_ENV=test nyc --all --show-process-tree --silent mocha test/*.js

With babel configuration like so:

  {"babel": {
    "presets": [
      "es2015"
    ],
    "env": {
      "test": {
        "plugins": [
          "istanbul"
        ]
      }
    }
  },
  "nyc": {
    "include": [
      "src/**/*.js"
    ],
    "exclude": [
      "node_modules"
    ],
    "require": [
      "babel-register"
    ],
    "sourceMap": false,
    "instrument": false
  }}

[queicherius]

I am possibly not doing it right, but the following command & .babelrc do not generate coverage for files that are not imported anywhere (but are included in the source directory):

NODE_ENV=test \
  nyc --all --include="src/**/*js" --sourceMap=false --instrument=false --reporter=lcov --reporter=text-summary --report-dir=coverage/ \
  mocha --compilers js:babel-core/register --require babel-polyfill tests/*

(I also tried --include="src/", which didn't work either.)

{
  "presets": ["latest", "stage-0"],
  "env": {
    "test": {
      "plugins": [
        ["istanbul", {"include": ["src/**"]}],
        "rewire"
      ]
    }
  }
}

The env of nyc looks like this:

{ NYC_CACHE: 'disable',
  NYC_CONFIG: '{"_":["/Users/david/Projects/scraping/node_modules/abc-environment/node_modules/.bin/mocha"],"silent":false,"s":false,"all":true,"a":true,"cache":false,"c":false,"check-coverage":false,"checkCoverage":false,"source-map":false,"sourceMap":false,"instrument":false,"hook-run-in-context":true,"hookRunInContext":true,"show-process-tree":false,"showProcessTree":false,"help":false,"h":false,"version":false,"include":["src/"],"n":"src/","reporter":["lcov","text-summary"],"r":["lcov","text-summary"],"report-dir":"coverage/","reportDir":"coverage/","exclude":["test/**","test{,-*}.js","**/*.test.js","**/__tests__/**","**/node_modules/**"],"x":["test/**","test{,-*}.js","**/*.test.js","**/__tests__/**","**/node_modules/**"],"require":[],"i":[],"extension":[],"e":[],"branches":0,"functions":0,"lines":90,"statements":0,"cwd":"/Users/david/Projects/scraping","$0":"/Users/david/Projects/scraping/node_modules/abc-environment/node_modules/.bin/nyc","instrumenter":"./lib/instrumenters/noop"}',
  NYC_CWD: '/Users/david/Projects/scraping',
  NYC_ROOT_ID: '135f9ea6c441b825034943c4b7aeff0f',
  BABEL_DISABLE_CACHE: 1,
  NYC_INSTRUMENTER: './lib/instrumenters/noop' }

[bcoe]

@queicherius could I get you to push this testing branch somewhere?

[queicherius]

@bcoe Sure, here you go.

[jomasti]

From a quick test of the repo @queicherius put up, if I pass --require babel-core/register to nyc, it works. So does that mean if we are already using babel-register with mocha, we have to add it again for use with nyc?

[simonbuchan]

I found for amazon-archives/amazon-cognito-identity-js#154 I had to add the following:

--- i/package.json
+++ w/package.json
@@ -69,7 +69,12 @@
     "timeout": "30s"
   },
   "nyc": {
+    "all": true,
     "cache": true,
+    "include": "src",
+    "require": [
+      "babel-register"
+    ],
     "sourceMap": false,
     "instrument": false
   }

This is a pain as nyc requiring babel-register as well as ava causes it to run quite a bit slower and triggers warning messages no matter what src / include / exclude rules I tried:

% npm run coverage

> [email protected] coverage /mnt/c/code/amazon-cognito-identity-js
> cross-env BABEL_ENV=nyc nyc --reporter=text --reporter=lcov ava

[BABEL] Note: The code generator has deoptimised the styling of "/mnt/c/code/amazon-cognito-identity-js/dist/aws-cognito-sdk.js" as it exceeds the max of "100KB".
[BABEL] Note: The code generator has deoptimised the styling of "/mnt/c/code/amazon-cognito-identity-js/dist/aws-cognito-sdk.min.js" as it exceeds the max of "100KB".

  79 passed

--------------------------|----------|----------|----------|----------|----------------|
File                      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------------|----------|----------|----------|----------|----------------|
All files                 |    52.45 |     48.8 |    50.44 |    52.45 |                |
 AuthenticationDetails.js |        0 |        0 |        0 |        0 |... 31,38,45,52 |
 AuthenticationHelper.js  |        0 |        0 |        0 |        0 |... 302,305,309 |
 CognitoAccessToken.js    |        0 |        0 |        0 |        0 | 28,35,42,43,45 |
 CognitoIdToken.js        |        0 |        0 |        0 |        0 | 28,35,42,43,45 |
 CognitoRefreshToken.js   |        0 |        0 |        0 |        0 |          26,33 |
 CognitoUser.js           |    70.73 |    63.91 |    75.38 |    70.73 |... 5,1153,1154 |
 CognitoUserAttribute.js  |        0 |        0 |        0 |        0 |... 60,61,68,75 |
 CognitoUserPool.js       |      100 |      100 |      100 |      100 |                |
 CognitoUserSession.js    |        0 |        0 |        0 |        0 |... 47,54,63,65 |
 DateHelper.js            |        0 |        0 |        0 |        0 |... 46,49,52,54 |
 index.js                 |      100 |      100 |      100 |      100 |                |
--------------------------|----------|----------|----------|----------|----------------|

That said, it does work, so 👍

[simonbuchan]

Ah, if I also add only to .babelrc it works silently:

--- i/.babelrc
+++ w/.babelrc
@@ -2,6 +2,9 @@
   "presets": [
     "es2015"
   ],
+  "only": [
+    "src"
+  ],
   "env": {
     "nyc": {
       "sourceMaps": "inline",

[wojtkowiak]

Works flawlessly, thanks @motiz88 !

[bcoe]

@queicherius from my testing, the problem was that babel-core/register needs to be required by nyc rather than by mocha:

  {"nyc": {
    "include": ["src/*.js"],
    "require": "babel-core/register"
  }}

--all gets executed by nyc before mocha is invoked, so I think this was ultimately an order of operations problem -- mind trying out this change and reporting back to me.

I've added a note that we should add this to our docs: #67

[motiz88]

There are two things I don't really get, which worries me:

1. Why would nyc requiring babel-register (aka babel-core/register) make any difference?
2. How come nyc --all works with babel-register, if the transpiled files are never saved to some dist/ folder before nyc runs?

My guess for 2, at least, is that it "works" because nyc is running into the Babel cache folder (which, I think, has recently been moved into ./node_modules/). There it reads whatever transpiled files happen to be left over from a previous run.

If this is what's at work here, then it seems rather brittle: users could move out the Babel cache, not have it populated at the time of running tests, or worse, have it populated with stale data / files they'd already deleted...

[queicherius]

@bcoe so from my testing

• It doesn't work without babel (as you wrote)
• It doesn't work if I require babel via nyc (and not mocha) and I use --include="src/"
• It doesn't work if I require babel via nyc (and not mocha) and I use --include="src/**/*.js"
• It does work if I require babel via nyc (and not mocha) and I use --only="src/"

[bcoe]

@queicherius this works for me:

{"scripts": {
    "test": "NODE_ENV=test $(npm bin)/nyc --all --include='src/' --sourceMap=false --instrument=false --reporter=text --reporter=text-summary --report-dir=coverage/ $(npm bin)/mocha tests/*"
  },
  "nyc": {
    "require": "babel-core/register"
  }}

Trying to reproduce your issue:

@motiz88 let me try to address your worries:

Why would nyc requiring babel-register (aka babel-core/register) make any difference?

When you run --all, all the files are walked in the context of the top-level nyc process; If you're using mocha to inject babel-core/register the automatic instrumentation will not take place until your test is executed in a new subprocess.

When --all is run, even though we're using a noop instrumenter, we still monkey-patch require such that the babel-register gets executed. We just ultimately don't return any code to execute, so counters aren't run:

    if (_this.fakeRequire) {
      return 'function x () {}'
    } else {
      return instrumented
    }

My guess for 2, at least, is that it "works" because nyc is running into the Babel cache folder

The babel cache is turned off at the top of nyc's bin, and in all subprocess that it executes -- I've found that it causes a lot of problems with test instrumentation.

[motiz88]

@bcoe Ohh, that makes sense. I was wondering about that "fake require" stuff.

[ghost]

Is there some concise guide on how to do this? My coverage takes ages now, but still doesnt deliver correct results.

[danez]

I tried to setup this for babel/babylon. I can get it work but with major performance problems and only by using babel-register.

Till now the setup looks like this:

1. precompile the src with babel + plugins-istanbul + rollup into a single file.
2. run tests with nyc and this config:

{
  "nyc": {
    "include": [
      "src/",
      "bin/"
    ],
    "sourceMap": false,
    "instrument": false
  }
}

So far so good, this generates coverage for all tested files and takes 8s to run 2327 tests.

Now i changed the config of nyc to

{
  "nyc": {
    "all": true,
    "include": [
      "src/",
      "bin/"
    ],
    "require": [
      "babel-register"
    ],
    "sourceMap": false,
    "instrument": false
  }
}

and run nyc with NODE_ENV=test.
This now includes uncovered files, but takes 58s which is +500% in runtime, and it also seems that it tries to transpile the rollup-bundle we generate before the test, because i see this message twice:
"[BABEL] Note: The code generator has deoptimised the styling of "/babylon/lib/index.js" as it exceeds the max of "100KB"."

So if I understand correctly there is currently no way to use --all without babel-register? I appreciate the work that has been done on this, but it seems a little bit to opinionated and complicated right now. Maybe we could have an option to tell the noop instrumenter where the instrumented code is in the case it is not generated on demand?

[bcoe]

@danez I know @motiz88 implemented this without the babel-plugin-istanbul use-case in mind, it simply looks for a coverage object in each file (and they can be pre-instrumented).

@motiz88 perhaps you could provide some instructions about how you would picture using --all and we can eventually translate these into a post on the docs site?

[simonbuchan]

@danez I ran into that too, I fixed it by adding an only key to my .babelrc for my source directory (which isn't ideal, but hey)

It looks like you will end up double-transpiling the sources you do load in tests, once for your pre-compile, once for your tests: to make it faster you could not use the transpile output for tests.

[justin-lau]

Probably a misconfiguration, but I still don't see all the files being covered with babel-plugin-istanbul.

// package.json

"devDependencies": {
    "babel-core": "^6.18.2",
    "babel-plugin-istanbul": "^3.0.0",
    "nyc": "^9.0.1" // nyc@next resolve to 9.0.1-candidate.1; also tried that
},
"babel": {
    "presets": ["es2015", "stage-2"],
    "comments": false,
    "env": {
        "test": {
            "comments": true,
            "sourceMaps": "inline",
            "plugins": ["istanbul"]
        }
    }
},
"nyc": {
    "all": true,
    "cache": true,
    "include": [
        "api/src/**/*.js",
        "web/src/**/*.js"
    ],
    "reporter": [
        "lcov",
        "text-summary"
    ],
    "require": [
        "babel-register"
    ],
    "sourceMap": false,
    "instrument": false
}

I see all the files if I take away babel-plugin-istanbul, turn on instrument and sourceMap, but the reported lines are inaccurate.

[hoschi]

Is there some pull request or other issue which tracks the "release state" of this issue? This issue is closed, but I can't find information if it is released or not and in which version.

[bcoe]

@hoschi @justin-lau @boui if you're continuing have issues with --all, perhaps you could open an issue and link an example project -- --all seems to have been working for the projects I've tested, but perhaps we're missing an edge-case.

[hoschi]

@bcoe is it released already? I got no answer to my last question.

[dietergeerts]

I still can't get this to work. I just installed all things, so they are at their latest version. Can someone please provide feedback on this? I just had 2 hellish days trying to get unit testing with jasmine to work with reporting and coverage.... this would be the last step.

[dietergeerts]

Any progress on this one?

[Aaronius]

I still don't understand how to get this working. Should this issue really be in a closed state?

[maple-leaf]

Please give a good documentation about this. Still don't know how to get --all works.

[bcoe]

@maple-leaf there's a link in the README to a slack channel; this might be a good place to field your question