ember-cli-code-coverage
ember-cli-code-coverage copied to clipboard
kategengler
Template coverage
@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
templateCoverageto 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
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
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.
@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
@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