Template coverage - #319

@rwjblue #103 seemed dormat for quite a while, so I took it up. Basically I'd,

• Rebased to master and refactored the code a bit.
• Handled branches and attribute nodes.
• Handled actions
• Added some tests
• Added include/exclude option

There are still some areas where some pointers would be helpful.

• How to introduce this change ? I've added it as an opt-in configuration templateCoverage to prevent breaking users' existing thresholds in CI.
• While this works for app, somehow it is not working for addon. The plugin in not invoked for addon files (though it is called for tests files). Am I missing anything here ?

Can you help me out ?

Coverage report of the test component

@rwjblue / @kategengler Any inputs on this ?

This would be great to merge!

I definitely want this feature. However, I don't know enough about this feature to evaluate the PR, maybe @rwjblue or @thoov (as a recent contributor), could review?

More than happy to rebase this PR or open up one with the fixes @thoov @rwjblue if needed

Can I help in anyway? This could be handy for us too.

bump - Really looking forward to this!

Also very interested in this feature! If needed, I'm happy to help get it over the finish line.

@kategengler I think we should consider just merging this and hoping for the best. I don't think anyone with deep knowledge of this work is around and able to review. Thoughts?

To note, tests don't seem to have run here, so we need to make sure CI runs and passes before merging.

@kategengler I think we should consider just merging this and hoping for the best. I don't think anyone with deep knowledge of this work is around and able to review. Thoughts?

To me, this is a scary statement because this means there's also nobody around with knowledge of this work to fix any bugs once released. I could be convinced if someone tries it out on a few apps, though.

@kategengler I think we should consider just merging this and hoping for the best. I don't think anyone with deep knowledge of this work is around and able to review. Thoughts?

To me, this is a scary statement because this means there's also nobody around with knowledge of this work to fix any bugs once released. I could be convinced if someone tries it out on a few apps, though.

@kategengler

I haven't had the chance to revist this so far.

I'll revist it again, this weekend, and maybe I can find what I am missing while running for addons.

Also, I need some clarity on how we will release this. Opt-in seems sensible, so I'll proceed with that. If needed we can change that.

I have few real world apps, engines and addons. So will share any findings with that as well.

Definitely opt-in at first, and then in the next major version or two, after refinement, make it opt-out. My team's apps would need a little work to get upgraded to the version this lands in, but after that I'd happily help report issues / attempt fixes.

Any updates on this?

@vstefanovic97 in light of your recent additions, perhaps you could pick this back up to get it over the finish line?

@RobbieTheWagner I can try :D, The code seems clear to me, but haven't tried to run this at all. I will make a fork an see how it goes, but probably won't be soon, but a slow effort on the side when I have some extra time.
I'll write updates here on how it's going next week

Would love this feature

@RobbieTheWagner I can try :D, The code seems clear to me, but haven't tried to run this at all. I will make a fork an see how it goes, but probably won't be soon, but a slow effort on the side when I have some extra time. I'll write updates here on how it's going next week

@vstefanovic97 any progress on this? I would love to help, but I don't think I have enough knowledge of how this works 😅

@RobbieTheWagner tbh I haven't had the time to tackle this at all right now...
Not sure when I will get to it :/
If you are interested in taking a stab at this maybe we can setup some time and we can go through the code together, and I can try to explain to you how it works and what the code is doing

@vstefanovic97 @mukilane would this support gts files if landed as is?

@SergeAstapov on first glance, I think it will require some extra work most likely, since probably lines, columns will be off

Folks,
I have ironed out some issues and simplified the setup.

Here is how it works
There is a htmlbars plugin template-instrumenter which will wrap the statements in the template with a helper (See how it transformes here https://astexplorer.net/#/gist/3e688037884cbb9ada53cda75c5cb56c/959c1ffa43e4f49639903aec0dbaf55bc8895bd4)

This plugin in included as a transform to babel-plugin-ember-template-compilation which is responsible for all the template compilation (from ember-cli-htmlbars >= v6.0.0) (afaik)

A coverage map is injected into the template module which will be incremented by the helper.

I have tested this against app, standalone engine, v1 addon,

Currently the following cases are yet to be covered.

1. gjs/gts support - While transformation happens, the line numbers are relative to the template tag, so source mapping for report is not working.
2. Embroider (apps/addons) - I haven't explored much on this and would probably take this up later
3. While all straighforward statements are covered, yield, modifiers might not be accurate enough. This can be improved incrementally.

How this can be released ?
Given the above cases, it is better to make this opt-in by enabling it coverage.js

// coverage.js

module.exports = {
  reporters: ['lcov', 'html', 'text', 'json-summary'],
  enableTemplateCoverage: true,
};

For addons, there would be slight tweak in index.

Old

included() {
  this._super.included.apply(this, arguments);
  this.options.babel.plugins.push(...require('ember-cli-code-coverage').buildBabelPlugin());
},

New

included() {
   this.options.babel = {
     plugins: [
       // eslint-disable-next-line node/no-extraneous-require
       ...require('ember-cli-code-coverage').buildBabelPlugin(),
     ],
   };

   this._super.included.apply(this, arguments);
}

Addon
App

I think template coverage would be fantastic but I am less than sure about merging something that only works for v1 app/addons at this stage. Over in #431 we are discussing vite-based embroider support.