ci: enforce Rust contract coverage - #595
Conversation
|
@Mojisola001-tech is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Mojisola001-tech Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
| working-directory: packages/contracts | ||
| run: >- | ||
| cargo llvm-cov | ||
| --package meridian-vault |
There was a problem hiding this comment.
adapter-common is missing from this --package list, so its coverage is never measured, confirmed directly in this run's own output: the printed table only has rows for blend-adapter/src/lib.rs, defindex-adapter/src/lib.rs, and vault/src/lib.rs, no adapter-common row at all. That crate has zero tests of its own (no #[cfg(test)] module) and still has two .unwrap()-based panics in require_vault_auth/get_usdc. #533's own motivation was closing exactly this kind of untested-panic gap in the code that custodies funds, so leaving out the one crate that's actually fully untested undercuts the point of adding the gate. Add --package adapter-common to the list.
| working-directory: packages/contracts | ||
| run: >- | ||
| cargo llvm-cov | ||
| --package meridian-vault |
There was a problem hiding this comment.
The --package list omits adapter-common, a workspace member (see packages/contracts/Cargo.toml) whose code is exercised by the other packages' tests but never measured or gated by this 97.5% threshold. Untested branches added to adapter-common can ship at 0% coverage while this gate still passes.
| @@ -151,6 +151,19 @@ jobs: | |||
| - name: Run contract tests | |||
| working-directory: packages/contracts | |||
| run: cargo test | |||
There was a problem hiding this comment.
This step and the new cargo llvm-cov step below both compile and run the full contract test suite, so every CI run now builds and tests the workspace twice, once uninstrumented here and once instrumented for coverage, for no extra signal since cargo llvm-cov already fails on a test failure too. Worth dropping this step in favor of the coverage one, or vice versa.
|
@Mojisola001-tech checking in, this has been sitting on REQUEST_CHANGES for over two days with no update. Let me know if you're still working through the feedback or need any help. |
|
@collinsezedike Apologies for the delayed update, and thank you for the clear feedback. I am actively working on both requested changes now: adding adapter-common to the coverage gate and removing the redundant uninstrumented contract-test run. I will post an update once the revised workflow has been verified. |
|
@collinsezedike Addressed in 22b32af: adapter-common is now included in the cargo llvm-cov package list, and the redundant standalone cargo test step has been removed because the coverage command runs the tests instrumented. Thanks again for catching both issues. |
collinsezedike
left a comment
There was a problem hiding this comment.
Both prior findings are fixed: adapter-common is now included in the --package list and shows real, non-zero coverage (confirmed in the CI log, 92.59% from indirect exercise via the other crates' tests), and the redundant cargo test step is gone, replaced entirely by the single cargo llvm-cov step. Merging now.
Overview
Adds measured Rust contract coverage to Meridian's existing contracts CI job.
Related Issue
Closes #533
Changes
Contract coverage gate
cargo-llvm-covandllvm-tools-previewin the contracts workflow.meridian-vault,meridian-blend-adapter, andmeridian-defindex-adapter.Verification Results