chore: add coverage report targets to Makefile#57
chore: add coverage report targets to Makefile#57drmingdrmer merged 5 commits intodatabendlabs:mainfrom
Conversation
The applier module had no dedicated test coverage. These 41 tests exercise every public and internal code path: entry type dispatch (Blank/Membership/Normal), all `Cmd` variants (AddNode, RemoveNode, SetFeature, UpsertKV, Transaction, KvTransaction), branch selection with first-match-wins semantics, recursive predicate evaluation (And/Or short-circuit, vacuous truth), condition evaluation against Seq/Value/KeysWithPrefix operands, all six compare operators, and every operation type (Get, Put, Delete, DeleteByPrefix, FetchIncreaseU64, PutSequential) including `match_seq` guard paths. Key design choice: tests call `a.commit()` before verifying via `sm.get_maybe_expired_kv()` because the applier writes to an in-memory view that only becomes visible to the `SMV003` after commit.
Adds 17 new integration tests covering all operations in the
`KvTransaction` gRPC API: `Put`, `Get`, `Delete`, `DeleteByPrefix`,
`FetchIncreaseU64`, and `PutSequential`. Tests verify exact response
values using single struct comparisons rather than field-by-field
assertions, matching the test assertion style rule added to CLAUDE.md.
Changes:
- Test `Put` with/without `match_seq`, new key, and response shape
- Test `Get` on existing and missing keys
- Test `Delete` with matching/mismatching seq; verify `prev_value` is
populated even on failed deletes
- Test `DeleteByPrefix` response `count` field
- Test `FetchIncreaseU64`: from zero, accumulation, floor (`max_value`),
`fetch_max_u64`, and `match_seq` match/mismatch
- Test `PutSequential`: key format, seq numbering, unique key generation
- Test branch predicates: AND, OR, `keys_with_prefix`, multi-branch
fallthrough, and no-match case
- Use `assert_eq!(*resp, pb::SomeType { ... })` for full struct
comparison instead of asserting individual fields separately
The server sets `proposed_at_ms` (a wall-clock timestamp) in `KvMeta`
on every write — both `upsert_kv` and `TxnOp::put`. Comparing
`pb::SeqV` or `databend_meta_types::SeqV` against a literal with
`meta: None` therefore always fails at runtime even though seq and data
are correct.
Fix by normalizing the actual response before asserting: clone the
outer struct (`TxnPutResponse` / `TxnDeleteResponse`) and strip meta
from any embedded `SeqV` using struct-update syntax
(`pb::SeqV { meta: None, ..v }`). The same normalization is applied to
`get_kv()` results via `.map(|v| SeqV { meta: None, ..v })`.
Add `cargo-llvm-cov` to the `setup` target and two new targets for
generating code coverage reports using `cargo llvm-cov nextest`.
Changes:
- Add `coverage` target for terminal summary output
- Add `coverage-html` target that writes `target/llvm-cov/html/index.html`
- Install `cargo-llvm-cov` via `make setup`
Example:
make coverage # print coverage summary
make coverage-html # open target/llvm-cov/html/index.html in browser
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet reviewed 4 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on drmingdrmer).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f06cec56a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR adds code coverage reporting capabilities to the Makefile and introduces comprehensive test coverage for the KvTransaction gRPC endpoint and the Applier state machine logic. The tests use a meta-stripping approach to handle non-deterministic proposed_at_ms timestamps in SeqV comparisons.
Changes:
- Added
cargo-llvm-covinstallation and coverage report targets to Makefile - Added 17 new integration tests for KvTransaction gRPC endpoint covering all operations (Put, Get, Delete, DeleteByPrefix, FetchIncreaseU64, PutSequential) and predicate logic
- Added 41 new unit tests for Applier state machine covering entry type dispatch, all Cmd variants, branch selection, predicate evaluation, and operation execution
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Makefile | Added cargo-llvm-cov to setup target and two new coverage targets (coverage and coverage-html) |
| crates/server/service/tests/it/grpc/metasrv_grpc_kv_transaction.rs | Added 17 comprehensive integration tests for KvTransaction gRPC endpoint with SeqV meta normalization |
| crates/server/raft-store/src/applier/mod.rs | Added conditional module declaration for applier_test |
| crates/server/raft-store/src/applier/applier_test.rs | New file with 41 unit tests covering all Applier state machine code paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A non-empty `Or` predicate that finds no matching child was returning `true` due to the loop fallthrough unconditionally returning `Ok(true)`. Only an empty `Or` (vacuous truth) should return `true`; a non-empty `Or` with all-false children must return `false`. Changes: - Fix `eval_predicate` `Or` arm: fallthrough now returns `Ok(children.is_empty())` - Update test assertion: `Or([eq_seq(99), eq_seq(88)])` → `false` - Simplify the speculative multi-line comment into a single-line description
810a3ad to
f34b04c
Compare
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on drmingdrmer).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changelog
chore: add coverage report targets to Makefile
Add
cargo-llvm-covto thesetuptarget and two new targets forgenerating code coverage reports using
cargo llvm-cov nextest.Changes:
coveragetarget for terminal summary outputcoverage-htmltarget that writestarget/llvm-cov/html/index.htmlcargo-llvm-covviamake setupExample:
test: strip proposed_at_ms from SeqV meta before struct comparison
The server sets
proposed_at_ms(a wall-clock timestamp) inKvMetaon every write — both
upsert_kvandTxnOp::put. Comparingpb::SeqVordatabend_meta_types::SeqVagainst a literal withmeta: Nonetherefore always fails at runtime even though seq and dataare correct.
Fix by normalizing the actual response before asserting: clone the
outer struct (
TxnPutResponse/TxnDeleteResponse) and strip metafrom any embedded
SeqVusing struct-update syntax(
pb::SeqV { meta: None, ..v }). The same normalization is applied toget_kv()results via.map(|v| SeqV { meta: None, ..v }).test: add comprehensive tests for KvTransaction gRPC endpoint
Adds 17 new integration tests covering all operations in the
KvTransactiongRPC API:Put,Get,Delete,DeleteByPrefix,FetchIncreaseU64, andPutSequential. Tests verify exact responsevalues using single struct comparisons rather than field-by-field
assertions, matching the test assertion style rule added to CLAUDE.md.
Changes:
Putwith/withoutmatch_seq, new key, and response shapeGeton existing and missing keysDeletewith matching/mismatching seq; verifyprev_valueispopulated even on failed deletes
DeleteByPrefixresponsecountfieldFetchIncreaseU64: from zero, accumulation, floor (max_value),fetch_max_u64, andmatch_seqmatch/mismatchPutSequential: key format, seq numbering, unique key generationkeys_with_prefix, multi-branchfallthrough, and no-match case
assert_eq!(*resp, pb::SomeType { ... })for full structcomparison instead of asserting individual fields separately
test: add comprehensive tests for
Applierstate machine logicThe applier module had no dedicated test coverage. These 41 tests
exercise every public and internal code path: entry type dispatch
(Blank/Membership/Normal), all
Cmdvariants (AddNode, RemoveNode,SetFeature, UpsertKV, Transaction, KvTransaction), branch selection
with first-match-wins semantics, recursive predicate evaluation
(And/Or short-circuit, vacuous truth), condition evaluation against
Seq/Value/KeysWithPrefix operands, all six compare operators, and
every operation type (Get, Put, Delete, DeleteByPrefix,
FetchIncreaseU64, PutSequential) including
match_seqguard paths.Key design choice: tests call
a.commit()before verifying viasm.get_maybe_expired_kv()because the applier writes to anin-memory view that only becomes visible to the
SMV003after commit.