Improve diskann-disk test coverage#1193
Conversation
Add 18 targeted unit tests covering previously untested error paths, control flow branches, and pure functions across 7 files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1193 +/- ##
==========================================
- Coverage 89.95% 89.86% -0.09%
==========================================
Files 489 488 -1
Lines 93127 93602 +475
==========================================
+ Hits 83772 84118 +346
- Misses 9355 9484 +129
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
arrayka
changed the title
There was a problem hiding this comment.
Pull request overview
This PR increases unit test coverage in the diskann-disk crate by adding targeted tests for previously untested branches and error paths across utilities, PQ scratch handling, sector graph reconfiguration, quantization generator validation, continuation helpers, and checkpoint record management.
Changes:
- Add unit tests for partition-count estimation edge cases (clamping, odd rounding, k_base multiplier).
- Add tests covering error-path and basic-success behavior in k-means clustering, PQScratch query validation, quant generator parameter validation, sector graph reconfiguration/defaulting, continuation processing behavior, and checkpoint invalidation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-disk/src/utils/partition.rs | Adds tests for estimate_initial_partition_count edge cases. |
| diskann-disk/src/utils/kmeans.rs | Adds tests for successful clustering output shape and cancellation error path. |
| diskann-disk/src/storage/quant/generator.rs | Adds test for missing compressed-file validation when resuming with offset. |
| diskann-disk/src/search/provider/disk_sector_graph.rs | Adds tests for reconfigure growth/no-op and block-size defaulting. |
| diskann-disk/src/search/pq/pq_scratch.rs | Adds tests for rejecting undersized queries and accepting oversized queries. |
| diskann-disk/src/build/chunking/continuation/utils.rs | Adds tests for early stop, yield-then-continue, and error propagation (sync/async). |
| diskann-disk/src/build/chunking/checkpoint/checkpoint_record_manager_with_file.rs | Adds tests for fresh-state behavior, missing-file completion state, and invalidation semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…crosoft/DiskANN into u/arrayka/diskann-disk-tests3
arkrishn94
left a comment
arkrishn94
left a comment
There was a problem hiding this comment.
Thanks Alex! Looks good to me.
| manager_b.get_resumption_point(WorkStage::Start)? | ||
| ); | ||
| // A different identifier should be independent | ||
| let manager_c = CheckpointRecordManagerWithFileStorage::new(&index_prefix, 99); |
There was a problem hiding this comment.
It's unclear to me how this asserts independence from a and b like the comment states.
| .to_str() | ||
| .unwrap() | ||
| .to_string(); | ||
| let manager = CheckpointRecordManagerWithFileStorage::new(&index_prefix, 999); |
There was a problem hiding this comment.
This appears to be the identical test to the last thing above?
| Some(0) | ||
| ); | ||
|
|
||
| clean_checkpoint_file(&index_prefix, identifier); |
There was a problem hiding this comment.
It's not a big deal, but part of point of tempdir() is that the object returned cleans up after itself on drop. This kind of clean is only really needed if you were doing multiple things in the same tempdir and wanted to clean in between.
| Box::new(tracker), | ||
| ); | ||
|
|
||
| assert!(result.is_ok()); |
There was a problem hiding this comment.
This line is redundant with the next one. If you match on result.unwrap() it will already panic if it's not ok.
| assert!(result.is_ok()); | ||
| match result.unwrap() { | ||
| Progress::Processed(idx) => { | ||
| assert_eq!(idx, 3); // stopped before processing item at index 3 |
There was a problem hiding this comment.
Is this comment correct? Processed(3) means processed to 3, or processed until 3? I really hope it is the former.
There was a problem hiding this comment.
Yes. it is the former. fixed.
| ); | ||
|
|
||
| assert!(result.is_ok()); | ||
| match result.unwrap() { |
There was a problem hiding this comment.
This is probably better written as an if let Progress::processed(idx) = result.unwrap() { } else { panic!(); }. I'm surprised clippy didn't warn on this; it is usually not happy with single arm match statements.
There was a problem hiding this comment.
Fixed. Even removed panic!() to increase code coverage number.
| Box::new(tracker), | ||
| ); | ||
|
|
||
| assert!(result.is_ok()); |
There was a problem hiding this comment.
Same comments from above apply here.
| ) | ||
| .await; | ||
|
|
||
| assert!(result.is_ok()); |
Adds 18 targeted unit tests to the
diskann-diskcrate covering previouslyuntested error paths, control flow branches, and pure functions.