ci(workflows): Add code coverage reporting for v2 #6884
Conversation
|
Review changes with Changed Files
|
AnuthaDev
force-pushed
the
code-vyapti
branch
from
8e2283a to
d91b994
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
AnuthaDev
force-pushed
the
code-vyapti
branch
5 times, most recently
from
d2a1842 to
4873fa2
Compare
AnuthaDev
force-pushed
the
code-vyapti
branch
from
c9cde82 to
e74b921
Compare
AnuthaDev
force-pushed
the
code-vyapti
branch
from
e74b921 to
a9ec871
Compare
| # - Explanation of ignore flags: | ||
| # - "*cargo*" : Exclude external crates from the generated `lcov.info` file | ||
| # - "target/*" : Exclude compile time generated .rs files, e.g. isodata.rs, chrono-tz/timezones.rs | ||
| # - "/*" : Exclude /Users/../rustlib/src/rust/library/core/src/panic.rs, /Users/../rustlib/src/rust/library/std/src/sys/thread_local/native/mod.rs etc. |
There was a problem hiding this comment.
The /Users paths are MacOS specific though, are they also needed on Linux (which is what CI runs on)?
There was a problem hiding this comment.
Or, we can use $HOME instead? I missed these during my review...
| # - Trying to generate branch coverage using "-Z coverage-options=branch" currently fails. Both grcov and cargo-llvm-cov crash when trying | ||
| # to process the generated `.profraw` files. | ||
| # - Explanation of ignore flags: | ||
| # - "*cargo*" : Exclude external crates from the generated `lcov.info` file |
There was a problem hiding this comment.
Would using *cargo* exclude paths like crates/router_env/src/cargo_workspace.rs in our repository as well? Ideally we'd want to include such paths in the coverage report.
This makes me wonder, should we consider passing something like --keep-only crates/* instead of --ignore, or are there anything outside of crates that we need included in the coverage report?
| - name: Process coverage report | ||
| if: ${{ env.RUN_TESTS == 'true' }} | ||
| run: | | ||
| grcov . --source-dir . --output-types lcov --output-path ${{ env.CODECOV_FILE }} --binary-path ./target/debug --ignore "*cargo*" --ignore "target/*" --ignore "/*" | ||
|
|
||
| - name: Upload coverage reports to Codecov | ||
| uses: codecov/codecov-action@v5 | ||
| if: ${{ env.RUN_TESTS == 'true' }} |
There was a problem hiding this comment.
Do we need coverage reports to be processed / uploaded for merge queue runs as well? If not, we'd have to tweak the if condition for both of these steps to not run on merge queue (but include the other conditions specified in RUN_TESTS).
| LLVM_PROFILE_FILE="coverage.profraw" target/debug/router | ||
| ``` | ||
|
|
||
| 3. Open a separate terminal tab and run the cypress tests, following the [README] |
There was a problem hiding this comment.
Nit:
| 3. Open a separate terminal tab and run the cypress tests, following the [README] | |
| 3. Open a separate terminal tab and run the cypress tests, following the [README][cypress-v2-readme] |
And update the link to be:
[cypress-v2-readme]: /cypress-tests-v2/README.md@pixincreate Should we consider linking to the v1 README file instead?
Should we also remove all content from cypress-tests-v2/README.md, and update it to suggest referring to the cypress-tests/README.md file, to ease maintenance?
There was a problem hiding this comment.
the thing is that, in v2, entire format / architecture is different from v1. and removing readme from might create some chaos imo.
i'm also planning to slowly start moving to ts in the coming days.
i prefer to keep it, at least for now.
| Note: | ||
| - It is necessary to stop the `router` process to generate the coverage file | ||
| - Branch coverage generation requires nightly and currently `grcov` crashes while trying to include branch coverage. (Checked using `--log-level` parameter in `grcov`) |
There was a problem hiding this comment.
Should this note have been included before the VS Code specific information?
There was a problem hiding this comment.
Also, please update the TOC.
Type of Change
Description
Added a CI pipeline that will run cypress tests and generate code coverage reports for v2
Additional Changes
Motivation and Context
Closes #1593
Closes #1587
Closes #1622
Closes #1700
How did you test it?
Checklist
cargo +nightly fmt --allcargo clippy