Bug 1690780 - Coverage support #1482
Conversation
.circleci/config.yml
Outdated
| name: Upload coverage report | ||
| command: | | ||
| sudo apt install python3-pip | ||
| pip3 install git+https://github.com/mdboom/glean_parser.git@coverage-analysis |
There was a problem hiding this comment.
This just installs a branch of glean_parser. No longer necessary once that branch is merged and we make a release.
.circleci/config.yml
Outdated
| sudo apt install python3-pip | ||
| pip3 install git+https://github.com/mdboom/glean_parser.git@coverage-analysis | ||
| glean_parser coverage --allow-reserved -c glean_coverage.txt -f codecovio -o codecov.json glean-core/metrics.yaml | ||
| bash <(curl -s https://raw.githubusercontent.com/mdboom/codecov-bash/master/codecov) -f codecov.json |
There was a problem hiding this comment.
Had to fork codecov.io's upload script so it wouldn't ignore yaml files...
|
initial commit that strips out yaml files in the uploader is here: codecov/codecov-bash@aa5f583 |
badboy
left a comment
badboy
left a comment
There was a problem hiding this comment.
This is ... too simple. :D
I guess the difference to the proposal is that we don't need to keep any state beyond the GLEAN_TEST_COVERAGE file part?
When we want to integrate this into test harnesses, we could just say "set this before you run your tests" or we can build out some helpers (such as our GleanTestRule) that sets that environment variable, right?)
glean-core/src/storage/mod.rs
Outdated
|
|
||
| storage.iter_store_from(metric_lifetime, &store_name, None, &mut snapshotter); | ||
|
|
||
| record_coverage(metric_id); |
There was a problem hiding this comment.
The proposal said snapshot_metric can be called from one non-test place. Do we need to handle this here?
Yeah, I think depending on the details, they could just set the environment variable before running the test harness (as here). I haven't tested that with Gradle/JUnit yet because my Android build environment seems to have broken... |
mdboom
force-pushed
the
coverage
branch
5 times, most recently
from
85baf49 to
60bbc85
Compare
|
The upstream changes in glean_parser and codecov.io bash uploader have landed, so this is ready for a final review. |
badboy
left a comment
badboy
left a comment
There was a problem hiding this comment.
Good from my side, couple of questions, mostly around documentation.
docs/dev/code_coverage.md
Outdated
| > which suggests it has a lower chance of containing undetected software bugs compared to a program with low test coverage. | ||
| > ([Wikipedia](https://en.wikipedia.org/wiki/Code_coverage)) | ||
| This chapter describes how to generate a traditional code coverage report over the Kotlin, Rust, Swift and Python code in the Glean SDK repository. To learn how to generate a coverage report about what metrics your project is testing, see the user documentation on generating testing coverage reports. |
There was a problem hiding this comment.
Should this link to the user documentation?
|
|
||
| A post-processing step is required to convert the raw output in the file specified by `GLEAN_TEST_COVERAGE` into usable output for coverage reporting tools. | ||
|
|
||
| This post-processor is available in the `coverage` subcommand in the [`glean_parser`](https://github.com/mozilla/glean_parser) tool. |
There was a problem hiding this comment.
Should this call out that (right now) we only support the codecov format?
Co-authored-by: Jan-Erik Rediger <badboy@archlinux.us>
2 participants
Just a playground to see if I can get the pieces working together.
Depends on mozilla/glean_parser#272 for the glean_parser side of things.
Example coverage uploaded to codecov.io here: https://codecov.io/gh/mozilla/glean/src/ed8704ac50bcdfcb44ddd3a1fd6004d0eef57675/glean-core/metrics.yaml
Unfortunately, this requires forking the codecov.io uploader, since it explicitly filters out coverage reporting on
yamlfiles. (In fairness "coverage" on yaml files is unusual, but seems like it shouldn't be out-and-out disallowed). Anyway, we might have to address that upstream in order for codecov.io to be a viable solution for our users of this feature.Other than that, the implementation here turned out much simpler than in the proposal (reminder to go back and update the proposal with where we ended up). By using only a environment variable to turn the feature on (thanks @brizental for the suggestion) and just writing to a file as we go (thanks @chutten for the suggestion), there's no need to hook into the unit testing framework, and thus there's no need for an FFI or language bindings, which makes this all over quite a small change.