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

Add Kover plugin and set up basic rules #35

Merged
merged 3 commits into from
Sep 12, 2022
Merged

Add Kover plugin and set up basic rules #35

merged 3 commits into from
Sep 12, 2022

Conversation

IgnatBeresnev
Copy link
Member

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 engine

Targeting 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)

@IgnatBeresnev IgnatBeresnev linked an issue Sep 5, 2022 that may be closed by this pull request
build.gradle.kts Outdated
Comment on lines 52 to 58
// JaCoCo engine instead of IntelliJ for better integration with codecov.io
engine.set(DefaultJacocoEngine)
rule {
bound {
minValue = 60
}
}
Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Sep 5, 2022

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.

Copy link
Contributor

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.

Copy link
Member Author

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 👍

Copy link
Contributor

@qwwdfsad qwwdfsad left a 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)

@qwwdfsad
Copy link
Contributor

👍

@qwwdfsad qwwdfsad merged commit 93850b2 into master Sep 12, 2022
@qwwdfsad qwwdfsad deleted the kover_report branch September 12, 2022 10:00
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.

Introduce Kover and setup rules and CI build properly
2 participants