test(drive): cover shared-prefix aggregate index insertion#3961
test(drive): cover shared-prefix aggregate index insertion#3961thephez wants to merge 2 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 (2)
📝 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 bd2bd25) |
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']
| 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']
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.
What was done?
insert_contract/v0/tests.How Has This Been Tested?
Targeted command:
cargo test -p drive shared_prefix_aggregate_index_combinations_reject_or_insert -- --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