Coverage minima: add more fine-grained control #253
Conversation
kitbellew
force-pushed
the
covmin
branch
3 times, most recently
from
cb5abca
to
1f25437
Compare
|
@gslowikowski Can you review this PR? |
|
Give me some days |
|
@gslowikowski when you review this change, do consider the companion scoverage/scoverage-maven-plugin#55 |
|
@gslowikowski friendly reminder to review. |
|
@kitbellew I remember about your PRs. I like them very much. It's great that you prepared two pulls for SBT and Maven at the same time. I've never used JaCoCo before (I used Cobertura for Java). Needed some time to learn JaCoCo and their Gradle, Maven and SBT plugins. I must admit, I like the rules idea: In SBT the JacocoThresholds class is too limited IMO I'm thinking about next step. Some time ago I was asked about the possibility to set different (higher) coverage minimum values for some stable and well tested packages, files or classes to avoid accidental lowering their coverage. This is not the topic for your PRs, but I think it's a good idea for future enhancement. We have to extend Scoverage plugins APIs carefully to be always backward compatible. I was busy recently, and I didn't want to merge your pulls and then think what to do next. Please, give me some more time. You could think about the more detailed checks I wrote about. Maybe it should be similar to rules in JaCoCo Maven plugin. Definitely I don't like SBT configuration. There is only very simple JacocoThresholds class. The field names are not clear for me. E.g. what does To sum up, give me some more time. I want to test your proposals and build some projects with JaCoCo for comparison. |
|
How about by glob filepath also like jest? http://jestjs.io/docs/en/configuration.html#coveragethreshold-object . |
|
Sorry to be that guy but is there any progress on this? As i would love to have branch minimum on my project (cant use forked code on my project) so would love this to be merged |
|
@gslowikowski are you interested in pursuing this still? |
|
Also hate to be that guy, but would love to have branch minimum, any progress on this PR? |
|
So I'm currently in the process of getting better acquainted with scoverage @mnotti, so before I review this I need to get a better overarching picture first. I'll then come back and revisit this. |
@ckipp01 didn't realize you've taken over these repositories now... I've been using a locally built maven version for the last 3 years so that definitely works. |
😬 yes, slowly reviving it.
Ah ok cool, this is great to hear. Are you able to rebase this, and then I'll take a closer look? |
will do |
|
@ckipp01 rebased. no changes, no conflicts. |
|
@ckipp01 should we try rerunning the build? perhaps the error was unrelated to the change. |
There was a problem hiding this comment.
Thanks a lot for your work on this @kitbellew. I'm curious to get your opinions on the questions posed.
Along with existing statement minimum, add branch minimum. Also, include this pair of control at the package and file level.
There was a problem hiding this comment.
LGTM!
Thanks again for taking this across the finish line @kitbellew, especially seeing that you opened two years ago 😆
I'll start the release process with the compiler plugin in a bit, so if nothing goes wrong, you'll see this somewhat shortly.
|
BTW I know you sent in a pr with these changes to the maven plugin as well, but no promises on when I'll actually get to that since I don't use Maven at all, and the entire CI on over there needs to be updated etc. |
of course. let me know when you are ready. there are no tests in that repo, so you'd have to go by my word that it's been working well for us for the past 3 years. |
Along with existing statement minimum, add branch minimum. Also, include
this pair of control at the package and file level.