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

Replace Istanbul for instrumentation? #217

Closed
wants to merge 1 commit into from

Conversation

novemberborn
Copy link
Contributor

Issues like #198, #183 and #168 have caused me to reflect on nyc's use of Istanbul. I think we should replace Istanbul for our instrumentation purposes. To this end I've started Copenhagen. This PR is a proof-of-concept of integrating Copenhagen with nyc. You can find a sample project at https://github.com/novemberborn/nyc-copenhagen-poc.

Please read the Background section for context but the tl;dr is that Copenhagen will enable us to:

  • Instrument the new JavaScript syntax, rather than trying to map the coverage report
  • Fix stack traces for instrumented code
  • Support languages like TypeScript
  • Report on all files without having to execute them

Thoughts?

@sarbbottam
Copy link

Copenhagen

👍 I like the name!

@novemberborn
Copy link
Contributor Author

@bcoe would love your thoughts on this when you have a chance.

@bcoe
Copy link
Member

bcoe commented Apr 6, 2016

@novemberborn haven't missed this pull request; just want to collect my thoughts and give some good feedback (busy week).

@novemberborn
Copy link
Contributor Author

@bcoe no worries :)

@bcoe
Copy link
Member

bcoe commented Apr 9, 2016

@novemberborn I'm going to dig into this code more thoroughly this weekend, and play. But wanted to give you some initial feedback.

I think the idea of a competing code-coverage library that is suited to ES2015 is smart, and will help push forward both Istanbul and Copenhagen. I also feel, however, that Istanbul has a lot going for it:

  • a huge community.
  • a lot of time to learn from the community, and address interesting edge-cases.
  • some really solid developers supporting the project.

What I'd love to see is this:

  • abstract out nyc so that we could plug in an alternative code-coverage library.
  • work closely with the Istanbul community to figure out the path forward to creating a solution that has really rock-solid support for ES2015 features.

@novemberborn
Copy link
Contributor Author

I also feel, however, that Istanbul has a lot going for it

Absolutely! I think it does several things at once though:

  • Instrument code and collect usage data
  • Interpret coverage data
  • Generate various reports
  • Provide a CLI

Copenhagen only covers the first item, but with the right breakdown for integrating with tools like nyc and AVA. Plus it provides a way of instrumenting any source that compiles to JavaScript.

@bcoe
Copy link
Member

bcoe commented Apr 9, 2016

@novemberborn could we work on building out a suitable integration point in Istanbul, such that we could configure an alternative instrumenter?

@jamestalmage
Copy link
Member

I think #117 is a good place to start with this. If we modularize there various components of nyc, hopefully @novemberborn can use that to develop nyc-copenhagen (or whatever). I would certainly be excited to participate in both projects.

@novemberborn
Copy link
Contributor Author

could we work on building out a suitable integration point in Istanbul, such that we could configure an alternative instrumenter?

@bcoe, but we're essentially already doing that. nyc decides what files to instrument, calls out to Istanbul to do the actual instrumentation, stores the coverage data, and then combines it all so Istanbul can generate its reports.

I think #117 is a good place to start with this. If we modularize there various components of nyc, hopefully @novemberborn can use that to develop nyc-copenhagen (or whatever). I would certainly be excited to participate in both projects.

@jamestalmage yea I've thought about that. The reporting code that Copenhagen generates will be able to take input source maps into account, and then it won't be hard to map the raw usage data back to the source files. Of course you'd need to generate reporting code for the original sources.

There would have to be some glue code in nyc (mostly to map sources to the reporting code etc) but that'd be pretty minimal.

@bcoe
Copy link
Member

bcoe commented Apr 14, 2016

@novemberborn would definitely land this if you can get it to the point that tests pass, and we can swap in copenhagen is a drop in replacement for istanbul -- I think probably istanbul should be the default, and we'd point folks at copenhagen for ES2015?

@novemberborn
Copy link
Contributor Author

@bcoe cool! I'll have a think about structuring the required work across the various projects.

I think probably istanbul should be the default, and we'd point folks at copenhagen for ES2015?

Yea that'd be a good place to start. We'll have to see how Copenhagen shapes up.

@bcoe
Copy link
Member

bcoe commented May 1, 2016

@novemberborn closing this pull for now; waiting with bated breath for the drop in API replacement.

@bcoe bcoe closed this May 1, 2016
JaKXz added a commit to JaKXz/react-redux-starter that referenced this pull request May 8, 2016
But istanbul is still not instrumenting all sources, despite the `include-all-sources` option. Hopefully, this goes away when istanbul is replaced in istanbuljs/nyc#217
JaKXz added a commit to JaKXz/react-redux-starter that referenced this pull request May 8, 2016
But istanbul is still not instrumenting all sources, despite the `include-all-sources` option. Hopefully, this goes away when istanbul is replaced in istanbuljs/nyc#217
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.

4 participants