-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add Kover
plugin and set up basic rules
#35
Conversation
build.gradle.kts
Outdated
// JaCoCo engine instead of IntelliJ for better integration with codecov.io | ||
engine.set(DefaultJacocoEngine) | ||
rule { | ||
bound { | ||
minValue = 60 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JaCoCo
engine is required for integration with codecov, as IntelliJ engine does not seem to be supported (Kotlin/kotlinx-kover#16). The downside here is that we're not dogfooding our engine, and reported values of these two engines might differ.
As an alternative I can add two tasks: one for IntelliJ engine, and another one for JaCoCo, but it can lead to more inconsistencies.
Not sure what the best practices are for setting rule bounds, right now I set it to 60% line coverage, which should give 10-15% of headroom before build failures (currently it's at 70-75% lines covered, depending on the engine). This should both catch big PRs without tests and overtime degradation in coverage, but not be annoying.
Can add additional bounds for instructions/branches/etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest sticking to the default IDEA engine. Multiple reasons for that:
- AFAIR, only multiplatform projects are not supported by Kover in codecov. There are JVM-only projects that successfully leverage Kover and codecov
- The default engine is more precise when measuring Kotlin coverage, esp. in some tricky places such as inline functions. We would like to use these capabilities
IMO, if codecov is not working in this setup, my vote is for giving up the codecov until it's supported.
Not sure what the best practices are for setting rule bounds, right now I set it to 60% line coverage
The reasonable lower bound is somewhere around 80-90% depending on the tool and what is not covered (e.g. in coroutines it's mostly toString
implementations).
I would suggest picking 75 if tests pass, no need to set the bar any lower. Any new API additional will force authors to write proper coverage and eventually we'll be able to raise this bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, if codecov is not working in this setup, my vote is for giving up the codecov until it's supported.
Maybe I'm doing something wrong, but xml reports generated by IntelliJ engine are rendered as empty if uploaded to codecov. Removed JaCoCo engine and will postpone codecov then
I would suggest picking 75 if tests pass, no need to set the bar any lower. Any new API additional will force authors to write proper coverage and eventually we'll be able to raise this bar.
Set it to 75 (current line coverage is 77.8). My concern is that it might be impossible or not rational to cover 100% of lines for every PR/feature, while in other situations you actually cover more than just your changes, so it evens out over time.
Having the bound be too close to the current coverage might lead to situations where you'd need to write tests for some other parts of code (not directly changed by you) in order to have high enough coverage so it passes the build.
But I guess nothing is set in stone, so it can be adjusted if the need arises 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(obligatory comment to be able to finish review)
👍 |
Will add
koverReport
as a build step in TeamCity after this gets merged, and will add codecov.io integration later this week if everyone's fine with JaCoCo engineTargeting
master
to mitigate build failures after it's added as a step in TC (the alternative is branch-specific settings, but imho that overcomplicates things)