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

Template coverage #319

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mukilane
Copy link

In continuation of #103

@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

@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

Any updates on this?

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.

None yet

9 participants