test(drive): cover shared-prefix aggregate index insertion#3961
test(drive): cover shared-prefix aggregate index insertion#3961thephez wants to merge 4 commits into
Conversation
Adds a Drive e2e regression test for the accepted contract shape from #3960: a count+sum prefix index combined with a deeper range-countable index sharing that prefix. The test currently fails with the unsupported NotCountedOrSummed wrapping error, documenting the gap until validation or layout support is fixed.
Extends the shared-prefix aggregate index regression coverage to exercise count-only, sum-only, and count+sum parent value trees against plain, range-count, range-sum, and combined range count+sum child continuations. The matrix distinguishes compatible cases that must insert from unsupported layouts that should either be rejected during contract validation or supported by Drive insertion.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new end-to-end test module for insert contract v0 shared-prefix aggregate index layouts, wires it into the test suite, builds contracts from parent/child flag combinations, and exercises contract application plus document insertion across a case matrix. ChangesShared-prefix aggregate index coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 4c6620b) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only PR adds a regression matrix for shared-prefix aggregate index layouts in Drive. The test is intentionally committed in a known-failing state (6 of 12 cases fail today per the PR description, confirmed by Codex running the test), with no #[ignore] or #[should_panic] gating — merging as-is will break cargo test -p drive on the base branch. A secondary issue conflates two distinct failure modes into the same outcome variant, weakening the regression signal.
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs`:
- [BLOCKING] packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs:263-359: Test is committed in a known-failing state and will break CI
This is a plain `#[test]` with no `#[ignore]` or `#[should_panic]`, yet the PR description states the current result is failure and lists six cases that fail today (`count_parent_range_sum_child`, `count_parent_range_count_sum_child`, `sum_parent_plain_child`, `sum_parent_range_count_child`, `count_sum_parent_plain_child`, `count_sum_parent_range_count_child`). Codex independently ran `cargo test -p drive shared_prefix_aggregate_index_combinations_reject_or_insert` and confirmed it fails.
The match arm at line 349 unconditionally treats `SharedPrefixOutcome::InsertFailed` as a failure regardless of `must_insert`, so all six cases will produce `assert!(failures.is_empty(), ...)` failures the moment this lands on `v3.1-dev`. Every unrelated PR running the Drive test job will go red until the upstream fix lands.
The author calls this the "intended regression signal until the platform either rejects unsupported layouts during contract validation or supports them during Drive insertion" — but a green-required CI test is the wrong vehicle for that signal. Options: (a) gate with `#[ignore = "tracks #3960 ..."]`; (b) restructure so today's observed behavior is the *passing* state and the future fix flips the assertion; (c) land alongside the production fix so the matrix passes on merge.
A secondary structural note: even when this is unblocked, a single `#[test]` looping over 12 cases produces one combined failure message rather than per-case test failures. Splitting into one `#[test]` per case (mirroring `range_countable_index_e2e_tests.rs`) would localize future regressions.
- [SUGGESTION] packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs:211-220: `apply_contract` failure is conflated with schema-level `ContractRejected`
`insert_review_document_for_case` maps two structurally different failure modes onto the same `SharedPrefixOutcome::ContractRejected` variant: factory-level rejection from `build_review_contract` (the genuine "reject at contract creation" fix path) and runtime failure from `drive.apply_contract` (a storage-layer failure, not validation rejection).
The assertion at line 344 then treats `ContractRejected` as "OK, the contract was rejected at creation time" for `must_insert: false` cases. If a future change starts surfacing the bug at `apply_contract` time (e.g. Drive errors when materializing the index trees rather than during document insertion), this test will silently flip from red to green even though nothing has been fixed at the validation layer this PR is regression coverage for.
Add an `ApplyFailed(String)` variant and treat it like `InsertFailed` in the assertion so the diagnostic output identifies precisely which Drive phase broke.
| #[test] | ||
| fn shared_prefix_aggregate_index_combinations_reject_or_insert() { | ||
| let cases = [ | ||
| SharedPrefixCase { | ||
| name: "count_parent_plain_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::PLAIN, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_parent_range_count_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::RANGE_COUNT, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_parent_range_sum_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::RANGE_SUM, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_parent_range_count_sum_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::RANGE_COUNT_SUM, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_plain_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::PLAIN, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_range_count_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_range_sum_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::RANGE_SUM, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_range_count_sum_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT_SUM, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_plain_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::PLAIN, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_range_count_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_range_sum_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::RANGE_SUM, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_range_count_sum_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT_SUM, | ||
| must_insert: true, | ||
| }, | ||
| ]; | ||
|
|
||
| let failures = cases | ||
| .iter() | ||
| .filter_map(|case| match insert_review_document_for_case(case) { | ||
| SharedPrefixOutcome::Inserted => None, | ||
| SharedPrefixOutcome::ContractRejected(_) if !case.must_insert => None, | ||
| SharedPrefixOutcome::ContractRejected(error) => Some(format!( | ||
| "{}: compatible shared-prefix aggregate indexes were rejected: {}", | ||
| case.name, error | ||
| )), | ||
| SharedPrefixOutcome::InsertFailed(error) => Some(format!("{}: {}", case.name, error)), | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| assert!( | ||
| failures.is_empty(), | ||
| "shared-prefix aggregate index contracts must either reject unsupported layouts at \ | ||
| contract creation or allow document insertion; incompatible accepted layouts failed for:\n{}", | ||
| failures.join("\n") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Test is committed in a known-failing state and will break CI
This is a plain #[test] with no #[ignore] or #[should_panic], yet the PR description states the current result is failure and lists six cases that fail today (count_parent_range_sum_child, count_parent_range_count_sum_child, sum_parent_plain_child, sum_parent_range_count_child, count_sum_parent_plain_child, count_sum_parent_range_count_child). Codex independently ran cargo test -p drive shared_prefix_aggregate_index_combinations_reject_or_insert and confirmed it fails.
The match arm at line 349 unconditionally treats SharedPrefixOutcome::InsertFailed as a failure regardless of must_insert, so all six cases will produce assert!(failures.is_empty(), ...) failures the moment this lands on v3.1-dev. Every unrelated PR running the Drive test job will go red until the upstream fix lands.
The author calls this the "intended regression signal until the platform either rejects unsupported layouts during contract validation or supports them during Drive insertion" — but a green-required CI test is the wrong vehicle for that signal. Options: (a) gate with #[ignore = "tracks #3960 ..."]; (b) restructure so today's observed behavior is the passing state and the future fix flips the assertion; (c) land alongside the production fix so the matrix passes on merge.
A secondary structural note: even when this is unblocked, a single #[test] looping over 12 cases produces one combined failure message rather than per-case test failures. Splitting into one #[test] per case (mirroring range_countable_index_e2e_tests.rs) would localize future regressions.
| #[test] | |
| fn shared_prefix_aggregate_index_combinations_reject_or_insert() { | |
| let cases = [ | |
| SharedPrefixCase { | |
| name: "count_parent_plain_child", | |
| prefix_flags: IndexFlags::COUNT, | |
| child_flags: IndexFlags::PLAIN, | |
| must_insert: true, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_parent_range_count_child", | |
| prefix_flags: IndexFlags::COUNT, | |
| child_flags: IndexFlags::RANGE_COUNT, | |
| must_insert: true, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_parent_range_sum_child", | |
| prefix_flags: IndexFlags::COUNT, | |
| child_flags: IndexFlags::RANGE_SUM, | |
| must_insert: false, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_parent_range_count_sum_child", | |
| prefix_flags: IndexFlags::COUNT, | |
| child_flags: IndexFlags::RANGE_COUNT_SUM, | |
| must_insert: false, | |
| }, | |
| SharedPrefixCase { | |
| name: "sum_parent_plain_child", | |
| prefix_flags: IndexFlags::SUM, | |
| child_flags: IndexFlags::PLAIN, | |
| must_insert: false, | |
| }, | |
| SharedPrefixCase { | |
| name: "sum_parent_range_count_child", | |
| prefix_flags: IndexFlags::SUM, | |
| child_flags: IndexFlags::RANGE_COUNT, | |
| must_insert: false, | |
| }, | |
| SharedPrefixCase { | |
| name: "sum_parent_range_sum_child", | |
| prefix_flags: IndexFlags::SUM, | |
| child_flags: IndexFlags::RANGE_SUM, | |
| must_insert: true, | |
| }, | |
| SharedPrefixCase { | |
| name: "sum_parent_range_count_sum_child", | |
| prefix_flags: IndexFlags::SUM, | |
| child_flags: IndexFlags::RANGE_COUNT_SUM, | |
| must_insert: true, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_sum_parent_plain_child", | |
| prefix_flags: IndexFlags::COUNT_SUM, | |
| child_flags: IndexFlags::PLAIN, | |
| must_insert: false, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_sum_parent_range_count_child", | |
| prefix_flags: IndexFlags::COUNT_SUM, | |
| child_flags: IndexFlags::RANGE_COUNT, | |
| must_insert: false, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_sum_parent_range_sum_child", | |
| prefix_flags: IndexFlags::COUNT_SUM, | |
| child_flags: IndexFlags::RANGE_SUM, | |
| must_insert: true, | |
| }, | |
| SharedPrefixCase { | |
| name: "count_sum_parent_range_count_sum_child", | |
| prefix_flags: IndexFlags::COUNT_SUM, | |
| child_flags: IndexFlags::RANGE_COUNT_SUM, | |
| must_insert: true, | |
| }, | |
| ]; | |
| let failures = cases | |
| .iter() | |
| .filter_map(|case| match insert_review_document_for_case(case) { | |
| SharedPrefixOutcome::Inserted => None, | |
| SharedPrefixOutcome::ContractRejected(_) if !case.must_insert => None, | |
| SharedPrefixOutcome::ContractRejected(error) => Some(format!( | |
| "{}: compatible shared-prefix aggregate indexes were rejected: {}", | |
| case.name, error | |
| )), | |
| SharedPrefixOutcome::InsertFailed(error) => Some(format!("{}: {}", case.name, error)), | |
| }) | |
| .collect::<Vec<_>>(); | |
| assert!( | |
| failures.is_empty(), | |
| "shared-prefix aggregate index contracts must either reject unsupported layouts at \ | |
| contract creation or allow document insertion; incompatible accepted layouts failed for:\n{}", | |
| failures.join("\n") | |
| ); | |
| } | |
| #[test] | |
| #[ignore = "tracks #3960: shared-prefix aggregate index layouts are accepted at contract creation but currently fail during document insertion; un-ignore when contract validation rejects them or Drive supports them"] | |
| fn shared_prefix_aggregate_index_combinations_reject_or_insert() { |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 3ab8552 — Test is committed in a known-failing state and will break CI no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| fn insert_review_document_for_case(case: &SharedPrefixCase) -> SharedPrefixOutcome { | ||
| let drive = setup_drive_with_initial_state_structure(None); | ||
| let pv = PlatformVersion::latest(); | ||
| let contract = match build_review_contract(case.prefix_flags, case.child_flags) { | ||
| Ok(contract) => contract, | ||
| Err(error) => return SharedPrefixOutcome::ContractRejected(error), | ||
| }; | ||
| if let Err(error) = apply_contract(&drive, &contract) { | ||
| return SharedPrefixOutcome::ContractRejected(error); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: apply_contract failure is conflated with schema-level ContractRejected
insert_review_document_for_case maps two structurally different failure modes onto the same SharedPrefixOutcome::ContractRejected variant: factory-level rejection from build_review_contract (the genuine "reject at contract creation" fix path) and runtime failure from drive.apply_contract (a storage-layer failure, not validation rejection).
The assertion at line 344 then treats ContractRejected as "OK, the contract was rejected at creation time" for must_insert: false cases. If a future change starts surfacing the bug at apply_contract time (e.g. Drive errors when materializing the index trees rather than during document insertion), this test will silently flip from red to green even though nothing has been fixed at the validation layer this PR is regression coverage for.
Add an ApplyFailed(String) variant and treat it like InsertFailed in the assertion so the diagnostic output identifies precisely which Drive phase broke.
| fn insert_review_document_for_case(case: &SharedPrefixCase) -> SharedPrefixOutcome { | |
| let drive = setup_drive_with_initial_state_structure(None); | |
| let pv = PlatformVersion::latest(); | |
| let contract = match build_review_contract(case.prefix_flags, case.child_flags) { | |
| Ok(contract) => contract, | |
| Err(error) => return SharedPrefixOutcome::ContractRejected(error), | |
| }; | |
| if let Err(error) = apply_contract(&drive, &contract) { | |
| return SharedPrefixOutcome::ContractRejected(error); | |
| } | |
| fn insert_review_document_for_case(case: &SharedPrefixCase) -> SharedPrefixOutcome { | |
| let drive = setup_drive_with_initial_state_structure(None); | |
| let pv = PlatformVersion::latest(); | |
| let contract = match build_review_contract(case.prefix_flags, case.child_flags) { | |
| Ok(contract) => contract, | |
| Err(error) => return SharedPrefixOutcome::ContractRejected(error), | |
| }; | |
| if let Err(error) = apply_contract(&drive, &contract) { | |
| return SharedPrefixOutcome::ApplyFailed(error); | |
| } |
source: ['claude']
There was a problem hiding this comment.
Resolved in 3ab8552 — apply_contract failure is conflated with schema-level ContractRejected no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3961 +/- ##
==========================================
Coverage 87.17% 87.18%
==========================================
Files 2629 2632 +3
Lines 327221 327563 +342
==========================================
+ Hits 285265 285592 +327
- Misses 41956 41971 +15
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a Drive e2e regression test for shared-prefix aggregate index layouts (#3960). Both prior blockers are resolved at 4c6620b: the test is gated with #[ignore] referencing #3960, and ApplyFailed is now a distinct outcome variant treated as a real failure. Only two low-value nitpicks remain on the ignored test itself; no in-scope blockers or suggestions. Note: codex-general and codex-rust-quality failed with authentication errors, so this verification reflects claude-only coverage.
Coverage note: Claude general and Claude rust-quality ran; Codex general and Codex rust-quality did not run because ACP returned Authentication required. The verifier evaluated the available reviewer outputs and the CodeRabbit zero-finding context.
💬 2 nitpick(s)
| let factory = | ||
| DataContractFactory::new(PROTOCOL_VERSION_V12).expect("expected to create factory"); |
There was a problem hiding this comment.
💬 Nitpick: Mixed protocol versions: factory pinned to V12 while apply/insert use latest()
build_review_contract constructs DataContractFactory::new(PROTOCOL_VERSION_V12) (line 133), while apply_contract and insert_review_document_for_case use PlatformVersion::latest() (lines 206, 214). Once the latest protocol version moves past 12, the factory will be building contracts on an older protocol but applying them against a newer one, which can mask or change the behavior this matrix is intended to pin down. For a deterministic regression test, pin everything to one fixed version (or thread PlatformVersion::latest().protocol_version through the factory call). Not a defect today since the test is #[ignore]d, but it will silently lose regression value at the next protocol bump.
source: ['claude']
| let failures = cases | ||
| .iter() | ||
| .filter_map(|case| match insert_review_document_for_case(case) { | ||
| SharedPrefixOutcome::Inserted => None, | ||
| SharedPrefixOutcome::ContractRejected(_) if !case.must_insert => None, | ||
| SharedPrefixOutcome::ContractRejected(error) => Some(format!( | ||
| "{}: compatible shared-prefix aggregate indexes were rejected: {}", | ||
| case.name, error | ||
| )), | ||
| SharedPrefixOutcome::ApplyFailed(error) => Some(format!( | ||
| "{}: contract application failed: {}", | ||
| case.name, error | ||
| )), | ||
| SharedPrefixOutcome::InsertFailed(error) => Some(format!("{}: {}", case.name, error)), | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
💬 Nitpick: must_insert: false + Inserted is silently accepted
The matrix treats (must_insert: false, Inserted) as a pass, matching the PR's stated intent. However, when the #3960 fix lands and previously-unsupported layouts become insertable, the test stays green without prompting anyone to flip those cases to must_insert: true and remove #[ignore]. Consider emitting a diagnostic (e.g., a println! under --nocapture) when a must_insert: false case successfully inserts so the matrix is self-cleaning after the fix.
source: ['claude']
2 participants
Issue being fixed or feature implemented
Adds regression coverage for #3960.
A data contract can currently be accepted with shared-prefix document indexes where a shorter aggregate index and a deeper range index produce a Drive layout that cannot be materialized during document insertion. The reported shape is a
[resourceId]count+sum index combined with a[resourceId, rating]range-countable index.This PR adds Drive e2e coverage for that accepted-but-uninsertable contract shape and related wrapper-matrix variants. The matrix is marked
#[ignore]because it intentionally captures the current failing behavior and should not make unrelated Drive CI jobs red before the production fix lands.What was done?
insert_contract/v0/tests.apply_contractfailures distinct from contract creation rejection in the test diagnostics.How Has This Been Tested?
Default targeted command:
cargo test -p drive shared_prefix_aggregate_index_combinations_reject_or_insert -- --nocaptureCurrent result: passes with the regression matrix ignored, so normal CI remains green.
Manual diagnostic command:
cargo test -p drive shared_prefix_aggregate_index_combinations_reject_or_insert -- --ignored --nocaptureCurrent result: fails as expected, listing the accepted shared-prefix layouts that cannot be inserted today. The compatible control cases insert successfully.
Observed failing accepted layouts include:
count_parent_range_sum_childcount_parent_range_count_sum_childsum_parent_plain_childsum_parent_range_count_childcount_sum_parent_plain_childcount_sum_parent_range_count_childThese failures are the intended regression signal until the platform either rejects unsupported layouts during contract validation or supports them during Drive insertion.
Breaking Changes
None. This PR adds test coverage only.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit