Skip to content
This repository was archived by the owner on Dec 8, 2024. It is now read-only.

Instrument coffeescript #167

Closed
wants to merge 4 commits into from

Conversation

jdfree
Copy link

@jdfree jdfree commented Mar 18, 2014

This is my proposal to resolve #166. I compiled ibrik's instrumenter.coffee to coffee-instrumenter.js and made the necessary modifications to use it.

@jdfree
Copy link
Author

jdfree commented Mar 20, 2014

Ok, sorry about the initial issues, but I do think this one is worth adding.

@gotwarlost
Copy link
Owner

slammed at work. Please give me a few days to take a look at this. Thanks.

@dashed
Copy link

dashed commented Apr 2, 2014

Does this generate code coverage for mocha + coffeescript files?

@Albert-IV
Copy link

+1 for native CoffeeScript support.

@willhoag
Copy link

willhoag commented Apr 9, 2014

+1

@Raynos
Copy link

Raynos commented Apr 9, 2014

-1 This tool works just fine for javascript.

Compile your coffeescript to javascript before using istanbul.

coffee src/* -o lib && istanbul instrument --out lib-cov lib/*.js

@dashed
Copy link

dashed commented Apr 9, 2014

@Raynos maybe for smaller projects. But I think the compilation step is a huge performance hit for larger projects.

@Albert-IV
Copy link

@Raynos Not only that, reports on compiled CoffeeScript are obfuscated.

There are source maps, but as far as I know Istanbul doesn't use them.

@jdfree
Copy link
Author

jdfree commented Apr 11, 2014

If all your source is javascript before you run instrument, this doesn't change the behavior at all.

This made it much easier for me to do something like this: https://github.com/airportyh/testem/tree/master/examples/coverage_istanbul

@nerdgore
Copy link

@Raynos As long as Istanbul is not using source maps this won't work for projects with more than a handful of files.

@beatport-james-free It may not change behaviour to compile your coffee files upfront, however it completely changes the structure of your code.

Consider this example:

    # This file has no branches!
    class Klass extends Entity  # Line 1 - Statement 1 + 2

      constructor: () ->        # Line 2 - Statement 3
        @property = 0           # Line 3 - Statement 4
      method: () ->             # Line 4 - Statement 5
        @property += 1          # Line 5 - Statement 6

Running these tests I would expect 100% coverage:

    describe "A coffescript class", ->
      it "should be instantiated", ->
        expect(new Klass).toEqual jasmine.any Object

    describe "The instance method method()", ->
      it "should increase property by 1", ->
        klass = new Klass
        klass.method()

        expect(klass.property).toEqual 1

However, since instrumentation happens after compilation, all the helpers the compiler adds are instrumented as well, basically screwing with all numbers.

Current output

================== Coverage summary ==================
Statements   : 100% ( 20/20 )
Branches     : 50% ( 1/2 )
Functions    : 100% ( 7/7 )
Lines        : 100% ( 7/7 )
======================================================

Expected output

================== Coverage summary ==================
Statements   : 100% ( 6/6 )
Branches     : 100% ( 0/0 )
Functions    : 100% ( 3/3 )
Lines        : 100% ( 5/5 )
======================================================

There is no good chance to add ignore comments too since the code is generated. To compile to JavaScript first will give you a better understanding of what is actually instrumented. Often it is not trivial to map this back to your source.

Obviously what is being instrumented is the compiled code:

    (function () {
      var Klass,
      // expanded helper code for readability
        __hasProp = {}.hasOwnProperty,
        __extends = function (child, parent) {
          for (var key in parent) {
            if (__hasProp.call(parent, key)) child[key] = parent[key];
          }

          function ctor() {
            this.constructor = child;
          }
          ctor.prototype = parent.prototype;
          child.prototype = new ctor();
          child.__super__ = parent.prototype;
          return child;
        };
        // expansion end - actually on 2 lines

      Klass = (function (_super) {
        __extends(Klass, _super);

        function Klass() {
          this.property = 0;
        }

        Klass.prototype.method = function () {
          return this.property += 1;
        };

        return Klass;

      })(Entity);

    }).call(this);

In a real world project this could be off by hundreds of lines and tens of branches.

So yes, +1 for native coffee support.

@jescalan
Copy link

+1 for coffee support!

@alinex
Copy link

alinex commented May 2, 2014

+1 for coffee support
+1 for source map support

@gicappa
Copy link

gicappa commented May 22, 2014

+1

@Lordnibbler
Copy link

what's going on with this PR? would love this feature

@mtscout6
Copy link

mtscout6 commented Jul 1, 2014

+1 for CoffeeScript support!

@mdlavin
Copy link

mdlavin commented Jul 8, 2014

I've fixed the merge issue and I've added coffeescript support to the 'cover' command (for use with grunt-mocha-istanbul task).

My code is available https://github.com/mdlavin/istanbul/tree/instrument-coffeescript . @beatport-james-free I'll open up a pull request against your branch so that the changes can make it into this pull request.

@gotwarlost
Copy link
Owner

OK, it's probably time I added my 2c to the conversation. I should probably add that I am not a coffee-script developer and have 0 real-world experience with it. :)

It seems to me that people want wildly different things that I'll try to summarize here:

  1. A way to preserve existing coffee-script coverage processing (broken as it might be) but make it more seamless so people don't have to jump thru hoops trying to get coverage reports
  2. Native coffee-script support in the sense of not instrumenting boilerplate code that the transpiler generates and actually covering coffescript statements/ expressions/ branches as first-class citizens
  3. istanbul to honor source maps which might have a pleasant side-effect of processing JS transpiled from coffeescript "correctly" (for some value of correctness)

Re: 1, if people really want it, I'm ok with adding it to the istanbul source tree provided

  • the JS version of the coffee instrumenter is checked into lib/vendor or something and there is a standard build process by which this file can be updated when there are changes to the .coffee version. In short, I don't want to see it as a first-class source file in istanbul, and do not want to be responsible for "maintaining" it.
  • the instrumenter changes do not have so many if/elses added to it. Make a change that maps instrumenter instances by file extension, lookup the instrumenter to be used based on extension and make the interfaces exposed by the 2 instrumenters as close as possible (preferably identical) and that will keep me happy.

Re 3, source maps may be a good solution for other use cases (e.g. browserify bundling etc.) but IMO, it is not a good solution for the "native coffee instrumentation" problem. Having checked some source maps that coffee script produces, it is basically mapping all the boilerplate into line 1, col 1 of the coffee source (I tried the class foo extends bar syntax). If all we did was add source map support to istanbul (which is something I still need to wrap my head around), you might be able to see some coffeescript in the html reports but a bunch of annotations that won't make any sense.

Re 2, "native coffee instrumentation", this is an interesting problem that would actually provide first-class support for cs. Similar to the way istanbul adds on nodes to the AST for JS, if we added similar AST nodes to the cs parse tree, it would be doable. Most of the stuff including coverage object format, reporting etc. would remain unchanged but you would be able to see and understand the HTML reports in a purely "native cs way".

In short the mechanism would be (at a very high level):

  • Take the coffee-script source tree, turn it into an AST and add additional nodes to it
  • Write back the AST into generated code that is still all in cs and then transpile it

I looked at how hard it might be and (given my limited understanding) it seems to be pretty hard. The main issues I see are:

  • Multiple compilers (original versus redux)
  • No formal docs on the parse tree (compare against the Mozilla Parser API for instance)
  • Subtle diffs in the way the redux and the mainline compiler work

I would like to hear thoughts from other folks. The way I see this PR, we are still in the "requirements gathering" phase :)

@arthurschreiber
Copy link

Hey @gotwarlost,

I know this issue has been dead for some time, but has there been any progress on this so far? If not, what would you think about tackling these problems in different ways?

I'm using traceur to compile javascript code from ES.next to the ES5, and execute that in node.js on-the-fly. For what it's worth, I have a custom instrumenter that performs this compilation, passes the code back to the default istanbul instrumenter, does all the normal istanbul stuff, and then at the end maps the coverage line numbers back onto the "source" version of the code using a sourcemap.

The output is not perfect, but definately "good" enough to give a good overview of the coverage in our code. I'd love to extract the source mapping logic of this instrumenter for other pre-compilers to use, and extend istanbul to support "pluggable" instrumenters.

Would you be open to accept patches for that?

I know this does not really solve the coffee-script issue, as you mentioned above that the coverage reporting generated by this output is less than optimal, but it'd be a first step. And having pluggable instrumentation would mean that people could actually use this as a starter for writing a "real" coffee-script instrumenter that does not rely on sourcemaps, but on instrumenting the coffeescript code itself.

@gotwarlost
Copy link
Owner

I'm not entirely sure pluggable instrumenters are the way to go. What about adding source-map support directly to the istanbul instrumenter?

I've been super busy in my day job and haven't had a chance to think about this for around 2 months now. The main problem from what I recall is that the instrumenter assumes one and one only source file in its data structures and source maps can break this assumption.

If we were to generalize the data structure so that multiple source files can be supported, this change is not very hard (famous last words :) )

I'm happy to have a detailed discussion on this.

@arthurschreiber
Copy link

Well, source map support in the default instrumenter is not the issue, I'm thinking more about how to design this in a way that makes sense. My current implementation does the traceur source translation in the instrumenter, because that's also the point where I have to get the source map and store it for later use. The compilation and retrieval of the source maps might be different depending on the type of compiler you use (coffeescript, typescript, traceur, etc.). That's why I though a pluggable instrumenter might be the best approach.

Anyway, I'll see if I can whip something up so we have something more concrete to reason about.

@gotwarlost
Copy link
Owner

Sounds good.

@hems
Copy link

hems commented Dec 14, 2014

go guys 👍 !

@silkentrance
Copy link

Have you taken a look at the coffee-coverage project?
It will instrument the coffee-script prior to that it will compile it to javascript so that there is actually no need for any source maps, except for a few edge cases, but I can live with that.

@taoeffect
Copy link

+1.

In the meantime, coffee-coverage has been working for me (with mocha too).

@alinex
Copy link

alinex commented Jun 26, 2015

As you mentioned above CoffeeCoverage really does the trick and works fine for me, too.

@gotwarlost
Copy link
Owner

closing this PR that has languished for so long. A lot of other-than-coffee use-cases are being currently solved with source maps (from the branch, 3rd party transformers etc.). I plan to bring some sanity to this soon. Adding a coffee script instrumenter directly to istanbul seems wrong in the first place.

@gotwarlost gotwarlost closed this Oct 14, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support instrumenting coffeescript