[TST] add proptest for config & schema reconciliation #5847
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This comment has been minimized.
This comment has been minimized.
jairad26
force-pushed
the
jai/schema-prop-tests-rust
branch
from
fe605a3 to
67d62e4
Compare
|
[TST] Introduce property-based tests for Collection config⇄schema conversion & reconciliation This PR is entirely test-focused. It adds comprehensive proptest suites that randomly generate CollectionConfiguration and CollectionSchema instances to verify two critical behaviors: (1) lossless conversion from configuration to schema via Key Changes• Added proptest as a dev-dependency and wired it into Affected Areas• rust/types/src/collection_configuration.rs This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| } | ||
|
|
||
| #[cfg(feature = "testing")] | ||
| mod proptests { |
There was a problem hiding this comment.
[BestPractice]
Great job on adding these comprehensive property-based tests! They significantly improve the coverage for the configuration logic.
I noticed that many of the proptest strategy helper functions are duplicated between collection_configuration.rs and collection_schema.rs. For example:
embedding_function_strategyspace_strategyinternal_hnsw_configuration_strategyinternal_spann_configuration_strategyinternal_collection_configuration_strategyknn_index_strategy
To keep the code DRY and make these test utilities easier to maintain, consider extracting them into a shared test utility module. You could create a new file rust/types/src/test_utils.rs and declare it in your lib.rs like #[cfg(feature = "testing")] pub mod test_utils;.
The new test_utils.rs file could contain all the shared strategies:
// rust/types/src/test_utils.rs
#![cfg(feature = "testing")]
use crate::collection_configuration::{...};
use crate::hnsw_configuration::Space;
use proptest::prelude::*;
// ... other imports
pub fn embedding_function_strategy() -> impl Strategy<Value = Option<EmbeddingFunctionConfiguration>> {
// ... implementation
}
// ... other shared strategiesThen, in your test modules, you can simply import them with use crate::test_utils::*;. This will remove the duplication and centralize the test data generation logic.
Context for Agents
[**BestPractice**]
Great job on adding these comprehensive property-based tests! They significantly improve the coverage for the configuration logic.
I noticed that many of the `proptest` strategy helper functions are duplicated between `collection_configuration.rs` and `collection_schema.rs`. For example:
- `embedding_function_strategy`
- `space_strategy`
- `internal_hnsw_configuration_strategy`
- `internal_spann_configuration_strategy`
- `internal_collection_configuration_strategy`
- `knn_index_strategy`
To keep the code DRY and make these test utilities easier to maintain, consider extracting them into a shared test utility module. You could create a new file `rust/types/src/test_utils.rs` and declare it in your `lib.rs` like `#[cfg(feature = "testing")] pub mod test_utils;`.
The new `test_utils.rs` file could contain all the shared strategies:
```rust
// rust/types/src/test_utils.rs
#![cfg(feature = "testing")]
use crate::collection_configuration::{...};
use crate::hnsw_configuration::Space;
use proptest::prelude::*;
// ... other imports
pub fn embedding_function_strategy() -> impl Strategy<Value = Option<EmbeddingFunctionConfiguration>> {
// ... implementation
}
// ... other shared strategies
```
Then, in your test modules, you can simply import them with `use crate::test_utils::*;`. This will remove the duplication and centralize the test data generation logic.
File: rust/types/src/collection_configuration.rs
Line: 1084
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?