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

Better ES2015 Support #266

Closed
wants to merge 2 commits into from
Closed

Better ES2015 Support #266

wants to merge 2 commits into from

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Jun 4, 2016

The Problem

nyc has several open issues related to ES2015 support:

#265
#264
#215
#239
tapjs/tapjs#267

My Goal

Let's work towards adding better ES2015 support to nyc:

  1. using this ticket as a catch-all, and the tests I've checked in as a starting point, let's add regression tests for some of the bad behavior that folks have seen.
  2. let's fix the behavior! I think this will require coordination between Istanbul and nyc.
    • some of the bad line # mappings might be able to be fixed in our source-map logic (CC: @novemberborn )
    • we might need to address some of the issues in the underlying Istanbul coverage library (CC: @gotwarlost).
    • could we have better native ES2015 support, what does this look like (CC: @JaKXz)
  3. let's better document best practices for using nyc with ES2015:
    • I think this should probably be a separate document in a docs folder in nyc. My feeling being that some of the issues folks run into with nyc/ES2015 are actually operator errors.

To get started, I would ask that folks who have run into issues submit pull requests with reproductions to this branch (which will act as a catch all for the time being).

We can build out some failing tests, and hopefully get things to a happier place.

CC: @jmdobry, @skozin, @xjamundx, @remy, @JaKXz

@jamestalmage
Copy link
Member

I tried your sample using nyc and __coverage__

https://github.com/jamestalmage2/nyc-es2015

__coverage__ handled your example like a champ.

I think it would be great if we could figure out a way to create pluggable coverage instrumentation. Even when istanbul fixes this issue, __coverage__ is still compelling for people already using babel-register to instrument sources (it eliminates the current print ast from transform 1 => parse code into ast wasted effort that currently happens).

@bcoe
Copy link
Member Author

bcoe commented Jun 5, 2016

@jamestalmage really slick, should we just document __coverage__ as our sanctioned way to handle ES2015, and perhaps add some targeted docs for this?

@dtinth anything we can do to make nyc integrate more tightly with coverage?

@dtinth
Copy link
Contributor

dtinth commented Jun 5, 2016

@bcoe You can use babel-core with babel-plugin-__coverage__ instead of istanbul.Instrumenter.

Make sure to set babelrc: false to make sure that Babel will only perform coverage instrumentation. Here’s an example usage:

const Babel = require('babel-core')
const result = Babel.transform(code, {
  babelrc: false,
  plugins: [ require('babel-plugin-__coverage__') ],
  filename: filename
})

The result is an ES6 { code, map, ast } with coverage instructions added.

@jamestalmage
Copy link
Member

I think it would be better to add first class support for coverage . Setting it up now is pretty convoluted, especially when it comes to include/exclude config.

@jamestalmage
Copy link
Member

Interesting consequence of naming it __coverage__. It's bold if you don't surround with back ticks

@bcoe
Copy link
Member Author

bcoe commented Jun 5, 2016

@jamestalmage @dtinth this was my thinking too, could we create a shim with coverage that is a drop in replacement for istanbu.Instrumenter, and then make the instrumenter a configuration option in nyc?

@jamestalmage
Copy link
Member

I think that's a good first step, but I'd love to generalize nyc further.

Someday, we are going to allow users to compile their sources in the main process in AVA. We currently do that for tests, and not loading up babel in each thread made a HUGE performance increase. The eventual plan is to analyze the dependency graph, much the same way browserify does, and precompile all the required sources for a test before forking. It should provide a huge speedup.

Once that is done, I would love to integrate a first class --coverage flag, much as node-tap does, but I would want to do the coverage instrumentation in the main thread using __coverage__. My hope is that it will make coverage blazing fast.

Additionally, I think it would be awesome if AVA eventually integrated coverage into it's smart --watch mode. Keeping coverage data from tests we decided not to rerun, and combining it with new coverage data from tests we did. That will require tracking which tests are causing which files to be written to disk, and selectively deleting or combining them.

Finally, I think @novemberborn has some interesting ideas with copenhagen, and I'm sure there will be other clever coverage instrumentation solutions in the future. It would be nice if nyc provided a generalized infrastructure that provided coverage implementors a convenient way to make their solutions handle process forking.

Short term it may be better to go with an easy win that just allows for __coverage__.

@bcoe
Copy link
Member Author

bcoe commented Jun 9, 2016

closing in favor of the work being done in #268

@bcoe bcoe closed this Jun 9, 2016
bcoe added a commit that referenced this pull request Jun 13, 2016
… coverage (#268)

* feat: make nyc behave better when used with babel-plugin-__coverage__ (see #266)

* feat: abstract test include/exclude logic into its own module

* fix: add text-exclude dependency

* fix: should have instrumenters in files list

* fix: fix remapping issue with source-maps

* fix: typo was causing argument not to propagate

* docs: add section introducing __coverage__ for ES6/ES7

* docs: a couple small edits
@JaKXz JaKXz deleted the es2015-testing branch January 10, 2019 01:05
@JaKXz JaKXz restored the es2015-testing branch January 10, 2019 01:05
@JaKXz JaKXz deleted the es2015-testing branch March 7, 2019 21:28
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 this pull request may close these issues.

3 participants