add codecov workflow using cargo-llvm-cov #2157
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to f2bdce4 in 1 minute and 46 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/coverage.yaml:22
- Draft comment:
Add a newline at the end of the file. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is asking for a newline at the end of the file, which is a common style guideline in many coding standards. However, it is purely informative and does not suggest a specific code change or improvement. It doesn't align with the rules for good comments as it doesn't address functionality or logic.
2. .github/workflows/coverage.yaml:13
- Draft comment:
Consider pinning the Rust version instead of updating to 'stable', to ensure consistent builds. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While version pinning can help with reproducibility, using 'stable' is a common and valid practice in Rust workflows. The stable channel provides tested, reliable releases. This is a code coverage workflow, not a release build, so minor version differences are unlikely to cause issues. The suggestion seems overly cautious and could make maintenance harder by requiring manual version updates. Version pinning could prevent unexpected issues if a new Rust release introduces changes that affect coverage reporting. Some organizations require strict version pinning for all CI. The benefits of pinning here are speculative - there's no evidence of actual problems. Using stable is a standard practice that balances reliability with maintainability. Delete the comment. Using 'stable' is a valid approach for this coverage workflow, and the suggestion to pin is overly cautious without clear benefits.
3. .github/workflows/coverage.yaml:10
- Draft comment:
Consider adding caching for dependencies to improve workflow performance. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_IqY7v7DquN7wXoNM
You can customize 
.github/workflows/coverage.yaml
Outdated
| - name: Install Rust | ||
| run: rustup update stable | ||
| - name: Install cargo-llvm-cov | ||
| uses: taiki-e/install-action@cargo-llvm-cov |
There was a problem hiding this comment.
Pin the cargo-llvm-cov action to a specific commit or tagged version for reproducible builds.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 2e3f232 in 1 minute and 4 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_PMluRhj41lkUPwLH
You can customize
.github/workflows/coverage.yaml
Outdated
| - name: Install cargo-llvm-cov | ||
| uses: taiki-e/install-action@cargo-llvm-cov | ||
| - name: Change directory to engine | ||
| run: cd engine |
There was a problem hiding this comment.
Changing directory with 'cd engine' does not persist to subsequent steps. Use the 'working-directory' option instead.
| run: cd engine | |
| working-directory: engine |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2ff753a in 1 minute and 6 seconds. Click for details.
- Reviewed
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/coverage.yaml:18
- Draft comment:
Good use of working-directory instead of 'cd engine'; ensure relative paths (like output file) are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .github/workflows/coverage.yaml:22
- Draft comment:
Updated file path to 'engine/lcov.info' correctly reflects the working-directory change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. .github/workflows/coverage.yaml:23
- Draft comment:
Add a newline at the end of the file for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_RgrJbi0OfoY3irGE
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e57fc9f in 59 seconds. Click for details.
- Reviewed
36lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/coverage.yaml:3
- Draft comment:
Ensure the removal of 'push' events is intentional; running coverage only on pull request might miss coverage on direct pushes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. engine/baml-lib/ast/src/parser/parse.rs:19
- Draft comment:
The added #[allow(dead_code)] suppresses warnings for debug-only pretty_print. Confirm this aligns with intended debug behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. engine/baml-lib/prompt-parser/src/parser/parse_prompt.rs:11
- Draft comment:
Adding #[allow(dead_code)] to pretty_print in debug_parser is appropriate; just ensure it’s intended for debug builds. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_UQeAyGkTnFx49qBp
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bfeb847 in 45 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/coverage.yaml:12
- Draft comment:
Ensure the 'setup-all' action installs the correct Rust version, as this now replaces the explicit 'rustup update stable' call. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that a specific action installs the correct Rust version. This is a request for confirmation or verification, which violates the rule against asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
Workflow ID: wflow_7893aFLq3gInFzcn
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a62e093 in 33 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/coverage.yaml:17
- Draft comment:
Removal of '--all-features' flag may omit coverage for tests relying on non-default features. Confirm if this change is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_ilrsPzqHqzNzgjsC
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 303421b in 1 minute and 36 seconds. Click for details.
- Reviewed
42lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/coverage.yaml:1
- Draft comment:
Removed the standalone coverage workflow – ensure the local action in .github/actions/coverage fully replicates these steps (e.g., setting the working directory to 'engine' and installing cargo-llvm-cov). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ez6GGsamIs0wBW23
You can customize
| @@ -222,7 +222,8 @@ jobs: | |||
| cargo build --package baml_cffi | |||
| cargo test --lib | |||
| working-directory: engine | |||
|
|
|||
| - name: Run Coverage | |||
There was a problem hiding this comment.
The 'Run Coverage' step lacks a condition. Consider adding an 'if: matrix.test-suite == "rust-unit"' clause if coverage is intended only for Rust tests.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 886d642 in 1 minute and 45 seconds. Click for details.
- Reviewed
25lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:188
- Draft comment:
Matrix updated to include 'rust-coverage'. Ensure that steps intended only for rust-unit tests continue to be gated (using if conditions) so they don’t run in the coverage job. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. .github/workflows/primary.yml:227
- Draft comment:
Good addition of the if-condition for the coverage step. Verify that the custom action in ./.github/actions/coverage is correctly configured to use cargo-llvm-cov and report data to Codecov. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the configuration of a custom action, which falls under the rule of not asking the author to double-check or ensure behavior. It doesn't provide a specific suggestion or point out a specific issue with the code.
Workflow ID: wflow_6Y8ojHDzVklrrKUf
You can customize
| @@ -223,6 +223,24 @@ | |||
| cargo test --lib | |||
| working-directory: engine | |||
|
|
|||
| - name: Install cargo-llvm-cov | |||
| if: matrix.test-suite == 'rust-coverage' | |||
| uses: taiki-e/install-action@cargo-llvm-cov | |||
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
|
|
||
| - name: Upload coverage to Codecov | ||
| if: matrix.test-suite == 'rust-coverage' | ||
| uses: codecov/codecov-action@v3 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 3dc412f in 1 minute and 51 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:231
- Draft comment:
Ensure the generated 'lcov.info' file is output in the expected location relative to the repository root. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. .github/workflows/primary.yml:237
- Draft comment:
Verify that the Codecov action 'files' input (engine/lcov.info) correctly references the generated coverage file. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_m14kX8BdyGMWga0h
You can customize
| @@ -223,6 +223,24 @@ | |||
| cargo test --lib | |||
| working-directory: engine | |||
|
|
|||
| - name: Install cargo-llvm-cov | |||
| if: matrix.test-suite == 'rust-coverage' | |||
| uses: taiki-e/install-action@cargo-llvm-cov | |||
There was a problem hiding this comment.
Pin the cargo-llvm-cov action to a specific version (not just a ref) to ensure reproducibility.
| uses: taiki-e/install-action@cargo-llvm-cov | |
| uses: taiki-e/install-action@v0.8.6 |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7e305d9 in 1 minute and 14 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:234
- Draft comment:
Removing --all-features may omit coverage for optional features. Confirm this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment points out a potentially valid technical concern - removing --all-features could mean some code paths aren't being tested. However, this violates our rules about speculative comments and asking for confirmation. The author likely had a good reason for removing this flag, and we don't have enough context about the codebase's optional features to know if this is actually an issue. I might be undervaluing the importance of comprehensive test coverage. Missing feature coverage could hide bugs. While coverage is important, the comment violates our rules by being speculative and asking for confirmation. We should trust the author's judgment unless we have strong evidence of a problem. Delete the comment. It's speculative and asks for confirmation, which violates our review rules. We don't have strong evidence that removing this flag is actually problematic.
Workflow ID: wflow_04bGcAD3tFYibGgj
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4e16466 in 1 minute and 33 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:234
- Draft comment:
Verify if '--lib' is intentional; '--workspace' spans all crates, which may be needed for complete coverage. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment asks to "verify" the intention, which violates our rules about not asking for verification. Additionally, while the technical suggestion about workspace vs lib coverage scope is valid, we don't have enough context about why the author changed it to --lib. They may have intentionally wanted to restrict coverage to just library code vs tests/binaries. Without more context about the codebase structure and coverage requirements, we can't be certain this is incorrect. The comment raises a valid technical point about coverage scope. The change could potentially reduce coverage reporting. However, the comment violates our rules by asking for verification, and we lack context to know if the reduced scope was intentional. Delete the comment since it asks for verification and we don't have strong evidence that the change is incorrect.
Workflow ID: wflow_cjRs3EXeqDmypgw6
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed d11baf7 in 1 minute and 49 seconds. Click for details.
- Reviewed
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:237
- Draft comment:
The commands to build 'baml_cffi' and configure LD_LIBRARY_PATH are duplicated between rust-unit and rust-coverage steps. Consider refactoring to reduce duplication. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_R5F1tFTxdpSpUBQb
You can customize
| if: matrix.test-suite == 'rust-coverage' | ||
| run: | | ||
| # Find Ruby library path and add it to LD_LIBRARY_PATH | ||
| RUBY_LIB_PATH=$(ruby -e 'puts RbConfig::CONFIG["libdir"]') |
There was a problem hiding this comment.
Ruby is invoked to set LD_LIBRARY_PATH but the 'rust-coverage' job doesn’t include a Ruby setup step. Ensure Ruby is available (or add a setup step) to avoid failures.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 67fcde8 in 46 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:211
- Draft comment:
Ruby setup now runs for both 'rust-unit' and 'rust-coverage'. Consider quoting the if expression for clarity, e.g., if: "matrix.test-suite == 'rust-unit' || matrix.test-suite == 'rust-coverage'". - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_KyU3L6LaGk60wxxy
You can customize
Important
Add Rust code coverage workflow using
cargo-llvm-covand suppress dead code warnings in debug builds.rust-coveragetotest-suitematrix inprimary.yml.cargo-llvm-covand run coverage forrust-coveragetest suite.#[allow(dead_code)]topretty_print()inparse.rsandparse_prompt.rsfor debug builds.This description was created by
for 67fcde8. You can customize this summary. It will automatically update as commits are pushed.