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

Code coverage ES6 #185

Closed
FuzzOli87 opened this issue Nov 10, 2015 · 29 comments
Closed

Code coverage ES6 #185

FuzzOli87 opened this issue Nov 10, 2015 · 29 comments
Labels

Comments

@FuzzOli87
Copy link

Hello,

You guys recommend to use NYC for coverage since it can handle spawned files. However, when testing since I have to transpile it first, it uses the transpiled code to test for coverage.

Is there a way to configure it so that it does not do this and so that it uses the ES6 code?

@jamestalmage
Copy link
Contributor

NYC relies on istanbul. See this issue: gotwarlost/istanbul#212

istanbul has a source-map branch that looks like it is getting pretty close, but looks like it doesn't play well with some reporters.

I have been told that integrating istanbul-combine solves the issue, but I never got around to it.

If you can't wait for istanbul and NYC to release source map support, I recommend trying to fork NYC to use the istanbul source-map branch. If you run into issues combining coverage files checkout istanbul-combine

@FuzzOli87
Copy link
Author

@jamestalmage

Interesting. I should of done a bit more research before asking but I was stuck in traffic and I had it in my head. I'll give it a spin and report back.

@novemberborn
Copy link
Member

There's also https://github.com/SitePen/remap-istanbul. This seems to work for me:

nyc --reporter=json npm test
mv coverage/coverage-final.json coverage/coverage.json
remap-istanbul -i coverage/coverage.json -o coverage/coverage.json
istanbul report lcov

@jamestalmage
Copy link
Contributor

@novemberborn With fb98d5d merged where do we stand on this?

Can this be closed, or is this a different issue?

@novemberborn
Copy link
Member

@jamestalmage it's a different issue. Code coverage is computed over the compiled output, so the stats will be wrong and it's hard to see which line in the original code was not covered. remap-istanbul uses source maps to compute the coverage of the original source.

The approach I mentioned above works for my project, which uses a watcher to transpile source files and write them to disk, with their source maps. That output is what's tested. It not quite a one-line though and it depends heavily on your project is set up.

This isn't an issue with AVA though.

@novemberborn
Copy link
Member

This isn't an issue with AVA though.

By which I mean, since AVA isn't responsible for the coverage reports, it's not a bug with the project. Workarounds could be explained (maybe I should do so! 😉) but it's fairly tricky.

@novemberborn
Copy link
Member

@mindmelting
Copy link

I think the latest release of nyc now works out the box... (or at least it does for me!)

@jamestalmage
Copy link
Contributor

@bcoe,
I want to make sure AVA is able to take advantage of the latest nyc source-map goodness. We are doing a couple things that I suspect may interfere. I'm just going to list them; If you can tell me if anything sounds problematic, I'd appreciate it.

  1. AVA transpiles test files only, and avoids babel-core/register by default. This allows users to write tests in ES2015, even when they prefer to keep their module code in ES5 (good for small modules where you don't want a build step). If users want transpile their main modules, they need to call the register hook themselves.
  2. We want to allow "helper" files in the test folder (utilities that are shareable between tests). Currently that just means, any file with a _ prefix. We will be hooking require to automatically transpile these as well (all other files will pass through unmodified by default). I see Babel/ES2015 Support istanbuljs/nyc#58 hooks require as well. I also saw your discussion here: allow libraries that override require to instrument babel code babel/babel#3062. Any pointers as I implement AVA's require hook so I don't clobber nyc source-map support?
  3. We also intend to implement transpilation caching. If a file has not changed since last run, we will reuse the old transpilation result. We will store cached results as files in a temp folder, named according to the hash of their contents (with periodic deletion of least recently used items). This means the transpiled source (and attached sourcemap) will likely not be where nyc expects to find it. Assuming nyc instruments coverage on top of transpiled files, and then uses source-maps after the fact to generate coverage for the original source after the fact, we would probably need a way to tell nyc where to find source-map information for a given file.

I look forward to your response. Thanks for your time, and for making nyc awesome!

@bcoe
Copy link

bcoe commented Nov 30, 2015

  1. nyc executes first, so the logic that sets aside Babel's require hook using getters/setters should also work for AVA:

https://github.com/bcoe/nyc/blob/master/index.js#L138

We might find that the nyc -> AVA -> babel-core/register interaction is an issue (perhaps nyc will need to store a stack of require hooks).

  1. here's the flow that nyc uses for Babel, I think we could probably use a similar approach:
  • nyc delegates to AVA's require hook which will return the transpiled code with a source-map appended.
  • nyc will, in turn, instrument this transpiled code with test coverage.
  • using the source maps, nyc will report coverage that references the original pre-transpiled code.
  1. If we implement things like we've outlined in 2., I think that only AVA's require hook will need to know about the cache. nyc will be collecting coverage that references the pre-transpiled files.

Let me know anything I can do to help you implement this, feel free to point me at any branches that are
works in progress.

@jamestalmage
Copy link
Contributor

Thanks for the quick response. We will follow up as we move those initiatives along.

@novemberborn
Copy link
Member

We might find that the nyc -> AVA -> babel-core/register interaction is an issue (perhaps nyc will need to store a stack of require hooks).

istanbuljs/nyc#65 takes care of that.

nyc delegates to AVA's require hook which will return the transpiled code with a source-map appended.

This might have to be documented better on nyc's side, I'll look into that.

That said, the test files don't need to be instrumented, the source files that are being tested do. It would help though if AVA's documentation could explain the requirements so users don't have to read up on nyc. I'll look into that when things settle with nyc.

@sparty02
Copy link
Contributor

sparty02 commented Dec 4, 2015

I tried nyc with ava and failed... 😢 I feel like I'm doing something really simple wrong.

Here's a snippet of my run scripts:

"scripts": {
    "test": "ava src/test/**/*.js",
    "coverage": "nyc npm test"
}

I ran npm run coverage and got back:

> nyc npm test

module.js:339
    throw err;
    ^

Error: Cannot find module '[ path to my package root ]\npm'
    at Function.Module._resolveFilename (module.js:337:15)
    at Function.Module._load (module.js:287:25)

Any chance someone on this thread could assist in some light guidance? I'd be happy to follow up with some README updates.

@jamestalmage
Copy link
Contributor

"scripts": {
-    "test": "ava src/test/**/*.js",
+    "test": "nyc --reporter=lcov ava src/test/**/*.js"
-    "coverage": "nyc npm test"
}

@sparty02
Copy link
Contributor

sparty02 commented Dec 4, 2015

@jamestalmage with that change, running npm test :

> nyc ava src/test/**/*.js

module.js:339
    throw err;
    ^

Error: Cannot find module '[ path to my package root ]\ava'
    at Function.Module._resolveFilename (module.js:337:15)
    at Function.Module._load (module.js:287:25)

@jamestalmage
Copy link
Contributor

have you installed ava locally?

npm install --save-dev ava

@sparty02
Copy link
Contributor

sparty02 commented Dec 4, 2015

Yeah, ava runs just fine by itself. Here is a snippet from my dev deps:

"devDependencies": {
    "ava": "^0.7.0",
    "nyc": "^4.0.1"
}

@jamestalmage
Copy link
Contributor

Huh,
Delete your node_modules folder then:

npm cache clean && npm install

@sparty02
Copy link
Contributor

sparty02 commented Dec 4, 2015

Tried that, no dice.... same error as above.... 😞 Here's the full stack trace:

PS [ path to my package root ]> npm test

> [my package name]@[my package version] test [ path to my package root ]
> nyc ava src/test/**/*.js

module.js:339
    throw err;
    ^

Error: Cannot find module '[ path to my package root ]\ava'
    at Function.Module._resolveFilename (module.js:337:15)
    at Function.Module._load (module.js:287:25)
    at Function.Module.runMain (module.js:467:10)
    at Function.runMain ([my user folder]\.node-spawn-wrap-14548-317d756fdb99\node:40:10)
    at Object.<anonymous> ([ path to my package root ]\node_modules\nyc\bin\nyc.js:17:6)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:467:10)

@novemberborn
Copy link
Member

@sparty02 what Node and npm versions are you running?

@sparty02
Copy link
Contributor

sparty02 commented Dec 7, 2015

@novemberborn
node v4.2.0
npm v3.5.2

@novemberborn
Copy link
Member

@sparty02 wait you're on Windows right? nyc's Windows support isn't quite there yet, we'll be tracking progress in istanbuljs/nyc#81.

@sparty02
Copy link
Contributor

sparty02 commented Dec 7, 2015

@novemberborn Ah, yep, I'm on Windows (at least part time)....that must be it then. I'll watch that issue for updates.... thanks!

@bcoe
Copy link

bcoe commented Dec 10, 2015

@sparty02 we've added better support for Windows subprocesses in [email protected]:

https://github.com/bcoe/nyc/blob/master/CHANGELOG.md

It might be worth giving this a shot. If you still run into problems, could you point me at a repo I can test against?

@sindresorhus
Copy link
Member

It might be worth giving this a shot. If you still run into problems, could you point me at a repo I can test against?

@sparty02 Did that solve your problem? If no, would you be able to provide something that reproduces it?

@sparty02
Copy link
Contributor

@bcoe @sindresorhus sorry guys, I spaced out on this. I'll give it a shot tomorrow morning and circle back to let you know.

@sparty02
Copy link
Contributor

@bcoe I pushed a sample repo at https://github.com/sparty02/nyc-ava-demo. I stripped it down to almost nothing, and am still hitting the issue, so it definitely seems environmental. Let me know if I can provide any more info!

@sparty02
Copy link
Contributor

sparty02 commented Jan 3, 2016

@bcoe @sindresorhus Just a heads up that I saw some activity over on nyc that was done in the past few days that appeared to (at least loosely) be related to better spawn support on Windows. Long story short, I just tried my use case again with [email protected] and [email protected] and it worked!

@AutoSponge
Copy link

@sparty02 I just set this up on a windows machine and it works but I couldn't use nyc npm test in my scripts. nyc ava works just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants