Skip to content

[WIP] Refactor to use babel-plugin-istanbul#141

Merged
RobbieTheWagner merged 16 commits intomasterfrom
spike-simpler-instrumentation
Jan 22, 2018
Merged

[WIP] Refactor to use babel-plugin-istanbul#141
RobbieTheWagner merged 16 commits intomasterfrom
spike-simpler-instrumentation

Conversation

@rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented Oct 2, 2017

This has some negative effects still:

  • Does not re-write the paths to match "real" on disk paths (I left an inline TODO and a stub function to make this "easy" to add back in)
  • Instruments test files (need to add them to an excludes list)

Even with these negative side-effects, it has massive upside:

  • Massively less overall code to maintain
  • Does not require us to parse babel config (and therefore avoids issues
    around parallelism in broccoli-babel-transpiler)
  • Significantly faster when used (e.g. no longer has to double parse and
    process files)
  • Loads istanbul configuration from .istanbul.yml file (very little documentation about this, but we are going through the default istanbul code path that loads config)

TODO:

  • Fix issue with source not being include in reports
  • Fix paths so they match with real paths on disk (generally needs to be done in the middleware section)

Robert Jackson added 2 commits October 2, 2017 08:51
This has some negative effects still:

* Does not re-write the paths to match "real" on disk paths
* Does not instrument dummy app files (I think)

Even with these negative side-effects, it has massive upside:

* Massively less overall code to maintain
* Does not require us to parse babel config (and therefore avoids issues
  around parallelism in broccoli-babel-transpiler)
* Significantly faster when used (e.g. no longer has to double parse and
  process files)
@rwjblue rwjblue force-pushed the spike-simpler-instrumentation branch from 0f94156 to 8ed98e2 Compare October 2, 2017 15:56
@theseyi
Copy link

theseyi commented Oct 9, 2017

Will this get merged in the near term? @rwjblue or are the issues mentioned above blockers?

);

reporter.addAll(config.reporters);
reporter.write(map);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this use the above config.coverageFolder?

@srini88
Copy link

srini88 commented Nov 14, 2017

Great to see this PR. This is kind of a blocker for us. Waiting for this to be merged :-)

@rwjblue
Copy link
Collaborator Author

rwjblue commented Nov 14, 2017

FWIW, there are still some issues with this approach to be ironed out (mentioned some of them in the TODO's in the description), but I don't have the bandwidth to continue pushing it forward.

Definitely would love someone to take a stab at test driving things and PR'ing fixes back to this branch...

@igbopie
Copy link

igbopie commented Nov 15, 2017

Hi! I have a question. Ember has the 'knowledge' how to build our code, isn't it possible to leverage that 'knowledge' and just use source maps to get the real source code?.

Thanks!

@RobbieTheWagner
Copy link
Collaborator

Thanks to the great work by @adamjmcgrath this is ready and working! There may still be some things we need to add support for, but I am going to merge this in. I will release a beta version, and it would be great if people could help test it out 😃

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.

7 participants