7.0.0-alpha4 + babel-plugin-istanbul breaks transpilation for test runner

[JaKXz]

Minimal reproduction: https://gist.github.com/JaKXz/9c9aa2c9dcc106e59939ec4b9f79c4f4

continued from: #288 (comment)

$ npm t

> nyc-ava-test@1.0.0 test /Users/Jason/Downloads/e91baeead7f4ff2df6825abf33327479-987c9806f48df16c90abf1c069044f2437551bbf
> cross-env NODE_ENV=test nyc -r lcov ava


   2 failed


   1. should default to hello world
   failed with "Cannot read property '0' of undefined"
      say (say.js:1:6143)
    Test.fn (test.js:5:8)
    processEmit [as emit] (node_modules/nyc/node_modules/signal-exit/index.js:146:32)
    processEmit [as emit] (node_modules/nyc/node_modules/signal-exit/index.js:146:32)
    _combinedTickCallback (internal/process/next_tick.js:67:7)
    process._tickCallback (internal/process/next_tick.js:98:9)


   2. should say a custom greeting to hello world
   failed with "Cannot read property '0' of undefined"
      say (say.js:1:6143)
    Test.fn (test.js:9:8)
    processEmit [as emit] (node_modules/nyc/node_modules/signal-exit/index.js:146:32)
    processEmit [as emit] (node_modules/nyc/node_modules/signal-exit/index.js:146:32)
    _combinedTickCallback (internal/process/next_tick.js:67:7)
    process._tickCallback (internal/process/next_tick.js:98:9)
File [/Users/Jason/Downloads/e91baeead7f4ff2df6825abf33327479-987c9806f48df16c90abf1c069044f2437551bbf/say.js] ignored, nothing could be mapped
npm ERR! Test failed.  See above for more details.

I am not fully sure which project to "blame", so I can move this to the babel-plugin-istanbul repo if that's better. @bcoe @gotwarlost cc @kentcdodds if you see this in your test as well.

[gotwarlost]

I did some experimentation. I can reproduce this issue. I suspect what is going on is that (given column 6143 and such) say.js is being processed by babel twice and something in there is causing the issue. Is there a way to print what babel plugins are being used?

I can only get columns > 5000 when I process say.js with the es2015 preset + istanbul plugin and repeat the process on the output file.

[bcoe]

@gotwarlost @JaKXz I see babel-register being required twice:

"ava": { "require": [ "babel-register" ] }, "nyc": { "require": [ "babel-register" ] },

mind trying this approach with babel-register only required by nyc?

[bcoe]

@gotwarlost @JaKXz okay, dug into this, the root problem seems to be that ava isn't finding the babel stanza in package.json when invoked with nyc:

I got things working by creating a .babelrc like so:

{
  "presets": ["es2015"],
    "env": {
      "test": {
        "plugins": [
          "istanbul"
        ]
      }
    }
}

And updating package.json to look like this (note that I've turned off processing of source-maps and instrumentation in nyc):

{
  "name": "nyc-ava-test",
  "version": "1.0.0",
  "main": "say.js",
  "scripts": {
    "test": "cross-env NODE_ENV=test nyc --no-cache -r text ava"
  },
  "license": "ISC",
  "ava": {
    "require": [
      "babel-register"
    ]
  },
  "nyc": {
    "sourceMap": false,
    "instrument": false
  },
  "devDependencies": {
    "ava": "^0.15.1",
    "babel": "^6.5.2",
    "babel-cli": "^6.9.0",
    "babel-plugin-istanbul": "^1.0.1",
    "babel-preset-es2015": "^6.9.0",
    "babel-register": "^6.9.0",
    "cross-env": "^1.0.8",
    "nyc": "^7.0.0-alpha.4"
  }
}

@jamestalmage, @novemberborn any thoughts about why we're not finding the babel stanza, nyc's configuration is finding the nyc stanza.

[novemberborn]

the root problem seems to be that ava isn't finding the babel stanza in package.json when invoked with nyc:

Given this AVA config:

  "ava": {
    "require": [
      "babel-register"
    ]
  },

AVA uses its default Babel config when transpiling test files. Source files are taken care of through babel-register.

Is nyc trying to instrument the test files? Cause they probably shouldn't be instrumented. Though you can use the babel config for AVA to change how it transpiles the test files.

---

P.S. @bcoe @gotwarlost I'm quite excited about this! 💯 Just been heads down with client work and preparing a conference presentation so haven't had a chance to chip in yet

[bcoe]

@novemberborn I turned off nyc instrumenting files, and got things working as long as I used a .babelrc rather than a babel line in package.json, it feels like it must be a path problem and find-up failing us in ava's context?

I did upgrade a few of the underlying libraries such as foreground-child so this could be a canary in the coal mine for a larger issue.

[bcoe]

@novemberborn @JaKXz I had stored a bad run in the cache, and babel config was being loaded correctly. The following configuration should work:

{
  "name": "nyc-ava-test",
  "version": "1.0.0",
  "main": "say.js",
  "scripts": {
    "test": "cross-env NODE_ENV=test nyc -r lcov ava"
  },
  "license": "ISC",
  "babel": {
    "presets": [
      "es2015"
    ],
    "env": {
      "test": {
        "plugins": [
          "istanbul"
        ]
      }
    }
  },
  "ava": {
    "require": [
      "babel-register"
    ]
  },
  "nyc": {
    "sourceMap": false,
    "instrument": false
  },
  "devDependencies": {
    "ava": "^0.15.1",
    "babel": "^6.5.2",
    "babel-cli": "^6.9.0",
    "babel-plugin-istanbul": "^1.0.1",
    "babel-preset-es2015": "^6.9.0",
    "babel-register": "^6.9.0",
    "cross-env": "^1.0.8",
    "nyc": "^7.0.0-alpha.4"
  }
}

If you're using babel-plugin-istanbul you must configure nyc to not handle source-maps or instrument code (this is handled by babel-plugin-istanbul instead:

 "sourceMap": false,
"instrument": false

[bcoe]

@JaKXz let me know if this fix does the trick for you; also if you can think of a better way to document this in the README let me know (give the version here a read if you don't mind: #286)

[gotwarlost]

@bcoe what exactly are the files that need to be deleted to start with a clean slate every time? I simply cannot correlate the changes I make to package.json with output that never changes. Once things start working they always work independent of what changes I make to package,json and vice-versa.

[bcoe]

@gotwarlost nyc populates files in node_modules/.cache, so that source-maps do not need to get parsed multiple times, and so that files don't need to get instrumented multiple times -- it seems to be this cache that's getting populated with bad data.

You can turn off this cache by passing the --no-cache flag to nyc.

[JaKXz]

@bcoe yeah we should definitely update the documentation for v7 with the instructions about --no-cache for the fix.

it seems to be this cache that's getting populated with bad data.

If nyc instrument and sourceMap set to false, though, should node_modules/.cache be populated with source mapped files at all?

[bcoe]

@JaKXz I believe this only bites you if you have data in the .cache from a failed run, going forward there's no need for the --no-cache flag.

[JaKXz]

@bcoe no, [that's why I mentioned it, because] after removing --no-cache after a successful run brings the same errors back.

[gotwarlost]

@bcoe it was really difficult for me to figure out what's going on with multiple modules maintaining caches in different (unexpected) places. At one point, I had 2 source trees that were checked out that were exactly the same, except one was working and another was erroring out. It wasn't obvious to me as to where I should look for a cache file.

I think we need to either document a mechanism or have a command to clear the cache, similar to npm cache clear.

Also, do we really need a cache?

[gotwarlost]

In other news, I published istanbul-reports@1.0.0-alpha.7. This fixes an issue where a branch is not considered for highlighting if there is only one path in the branch which is uncovered. This happens in the case of default assignments.

For example, if you comment out the test that uses the default object, you will get 3 uncovered branches - one each for each variable and one for the object itself, however the HTML report would not highlight anything as uncovered. alpha.7 fixes this problem with this commit.

istanbuljs-archived-repos/istanbul-reports@89294fe

[hoschi]

I had the same problem as @gotwarlost where even source trees from backup failed, which worked 3 days before, when setting up AVA together with nyc. Problem here is really babel has its own cache, ava maintains a cache and sometimes you need a 'disable all the performance and show me whats failing' flag.

[bcoe]

@jamestalmage and @novemberborn will attest to the fact that there's a pretty huge performance boost on a large codebase if cache is enabled (for consistency, AVA and nyc share the same underlying cache logic).

Having said this! now that we also have babel caching in the mix, maybe we should make nyc caching turned off by default? We can also try to work with babel to converge on a similar caching approach?

CC: @hzoo

[hoschi]

Don't get me wrong, performance is important! But bug tracing in this setup is really difficult. At the moment I e.g. have the problem that coverage reports are only right after the first run when I added a new test case. All following runs produce flaky results which are wrong. My first impression is a caching problem here, but its hard to check this easily at the moment with three places of caching.

we should make nyc caching turned off by default

the Readme says it is disabled by default, is this wrong?

[bcoe]

@hoschi, @gotwarlost, retracing my steps, I was considering turning the cache on by default in 7.x, it seems like keeping cache as an option that folks need to knowingly enable is the right call however.

[mourner]

@bcoe I think it's unfortunate that --cache is disabled by default. Is there a chance we could revisit this by making cache behavior more bulletproof? E.g. a lot of people that use tap will simply be unaware of the option because nyc is run under the hood.

Just got the unit tests in my projects down from 3m to 1m thanks to --cache — I wish I'd knew of the impact earlier.