-
Notifications
You must be signed in to change notification settings - Fork 74
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
Missing file coverage for Kotlin files with package mismatch #913
Comments
As a side note: while the bug reported above could be fixed on your end, there are some gray areas that lead to inaccurate codecov reports, but it's a bug of neither kover nor codecov, and more of a JaCoCo format's limitation. For instance, in Kotlin you can have multiple files with the same name if they are declared in different source sets or in different Gradle subprojects. This leads to codecov ignoring these files (presumably, it doesn't know which one to choose), but at the moment such files are indistinguishable in the report, so there doesn't seem to be an easy fix for it. For details, see Kotlin/kotlinx-kover#279 We (as in maintainers of Kover) are considering different options, ranging from enrichment of JaCoCo's format with custom tags (to provide an absolute path) to developing our own format altogether. However, for it to work, it would require external tools like codecov to support it. How open are you to customizing server-side parsers to match Kover's (enriched) reports, and to which extent? :) What would be the best line of communication to coordinate efforts? Email/Slack/GH issues/etc? |
@IgnatBeresnev I just saw and shared it with our Product team. |
Thanks! We are keeping track of this issue closely as it would enable codecov integration in official Kotlin libraries, as well as make codecov compatible with the project structure we are advocating for, so feel free to ask any questions |
@qwwdfsad Absolutely! The flat structure is something that customers have asked for, but Codecov currently can't support since we have no way to match the files in the reports to files in the repo. This would make a bunch of folks happy. |
@IgnatBeresnev What are the blockers to outputting absolute paths for files in the existing spec, rather then making changes that would require modifications to server-side and other tools in the community? |
Could you please elaborate on what exact spec do you mean? Is it correct that you would expect the following output for the example from the original report?
|
Codecov needs |
If we got it right, JaCoCo format spec has no notion of Let's take a look at the original report's data:
Here's the XML output:
Important note that JaCoCo XML format is not codecov-specific. It describes the overall coverage data for the given classes and packages and knows nothing about the actual filesystem layout. Moreover, other 3rd-party tools depend on that format and its properties' semantics, so it would be very much likely to break external tooling when making incompatible changes in such reports. It basically contains four entities:
So, TL;DR: we cannot change the meaning of any of the XML properties without breaking other tools' assumptions.
It would be nice if codecov could do the same. Alternatively, we can supply an additional XML-attribute somewhere with a relative path to a file in case of mismatch, if we will agree on that. Any other XML changes probably would also do the trick. That's quite a lot of text, so please let me know if it makes sense or if I'm missing or misunderstanding something |
I believe you are correct. I was thinking that Looking at our code, it appears we do the same thing as IDEA when it comes to crafting the file path:
I can create a feature request for this, i don't know what resources it would consume , given that we deal with some very massive monorepos and this map would either be huge, or need to be computed on each file. @aj-codecov, is #913 (comment) clear enough to copy over directly? |
Thanks!
It would be really nice.
The trick with this index is that it is not required at all if the class can be located at I believe that at some point, if the whole Kotlin ecosystem starts leveraging our "omitted common package prefix" scheme, we can come up with a better solution -- e.g. by adding a relative path to XML attributes. |
This is what we do now. We currently have no issues with Kotlin projects , only those that that already follow your "omitted common package prefix" scheme. You can view this here https://github.com/codecov/standards, as it runs daily. Did I misunderstood the ask / issue? |
I mean, the index is not required by default for all Kotlin projects; it only is necessary to build if the file does not exist at |
Describe the bug
In Kotlin it is possible to declare a class in a package that does not match file's actual location. For instance,
It will then result in the following
JaCoCo
-like XML report:As you can see, the path is taken from the
package
statement within the file, not file's location.This leads to codecov ignoring such files altogether. You can observe it in this report - the file should be present in the linked package, but it's not. GitHub link to the package
To Reproduce
See kover-reproducer project and its README.
Expected behavior
Codecov should calculate coverage for files in which package does not match the actual location.
Additional context
The report was generated by Kover: a Gradle plugin that calculates code coverage for Kotlin projects. It is maintained by the Kotlin team at JetBrains and is developed to analyze Kotlin code specifically.
Kover has the ability to generate
xml
reports that have the same structure as JaCoCo Java reports to enable compatibility with existing tools. At the moment, codecov's coverage for Kover's XML reports is inaccurate, some of the problems are Kover bugs, some are gray areas and some seem to be server-side codecov issues. Umbrella issue for codecov integration issues: Kotlin/kotlinx-kover#16. We're working on making the integration better for our users.cc @shanshin @qwwdfsad (maintainers of Kover)
The text was updated successfully, but these errors were encountered: