Skip to content

Template coverage#319

Open
mukilane wants to merge 1 commit intoember-cli-code-coverage:masterfrom
mukilane:template-coverage
Open

Template coverage#319
mukilane wants to merge 1 commit intoember-cli-code-coverage:masterfrom
mukilane:template-coverage

Conversation

@mukilane
Copy link

In continuation of #103

@mukilane mukilane force-pushed the template-coverage branch from feb14aa to f5e050a Compare June 12, 2021 15:17
@mukilane mukilane force-pushed the template-coverage branch 6 times, most recently from 49e59a3 to d89c5f2 Compare July 11, 2021 19:06
@mukilane
Copy link
Author

mukilane commented Jul 11, 2021

@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
image

@mukilane mukilane marked this pull request as ready for review July 17, 2021 08:59
@mukilane
Copy link
Author

@rwjblue / @kategengler Any inputs on this ?

@gabrielcsapo
Copy link

This would be great to merge!

@kategengler
Copy link
Collaborator

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?

@gabrielcsapo
Copy link

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

@mukilane mukilane force-pushed the template-coverage branch from d89c5f2 to 4e8dd66 Compare March 27, 2022 16:17
@Gorzas
Copy link

Gorzas commented Jul 13, 2022

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

@tehhowch
Copy link

tehhowch commented Jan 9, 2023

bump - Really looking forward to this!

@camerondubas
Copy link

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

@RobbieTheWagner
Copy link
Collaborator

@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?

@RobbieTheWagner
Copy link
Collaborator

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

@kategengler
Copy link
Collaborator

@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.

@mukilane
Copy link
Author

@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.

@tehhowch
Copy link

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.

@vstefanovic97
Copy link
Contributor

Any updates on this?

@RobbieTheWagner
Copy link
Collaborator

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

@vstefanovic97
Copy link
Contributor

@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

@kceb
Copy link

kceb commented Apr 23, 2024

Would love this feature

@RobbieTheWagner
Copy link
Collaborator

@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 😅

@vstefanovic97
Copy link
Contributor

@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

@SergeAstapov
Copy link
Contributor

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

@vstefanovic97
Copy link
Contributor

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

@mukilane mukilane force-pushed the template-coverage branch 8 times, most recently from d1cacd0 to 0a5b609 Compare July 29, 2025 09:32
@mukilane
Copy link
Author

mukilane commented Jul 29, 2025

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 image
App image

@mukilane mukilane force-pushed the template-coverage branch 2 times, most recently from 8c257ff to 960bdbf Compare July 29, 2025 15:16
@mukilane mukilane force-pushed the template-coverage branch from 960bdbf to c49a8dc Compare July 30, 2025 07:02
@kategengler
Copy link
Collaborator

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.

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.

10 participants