fix: unit tests for python values #452
Conversation
src/py/convert.rs
Outdated
| let int_value = value::Value::Basic(value::BasicValue::Int64(42)); | ||
| let int_type = schema::ValueType::Basic(schema::BasicValueType::Int64); | ||
| let py_obj = value_to_py_object(py, &int_value).unwrap(); | ||
| let roundtrip_value = value_from_py_object(&int_type, &py_obj).unwrap(); |
There was a problem hiding this comment.
value_from_py_object and value_to_py_object are functions internal to this module. Pythonized is the API exposed by the module. Ideally we want to test on the behavior of the public API.
You can also consider create a helper function as testutil to do this roundtrip conversion and validation (e.g. take a type + a value as input, do the roundtrip conversion internally and assert on equality), so that there will be less boilerplates in each specific test.
There was a problem hiding this comment.
Ah. Right. Thanks for pointing me in right direction!
| let roundtrip_value = value_from_py_object(&struct_type, &py_obj).unwrap(); | ||
| assert_eq!(struct_value, roundtrip_value, "Struct roundtrip failed"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can we also cover Table types? Thanks!
|
@badmonster0 can you please review this once? |
| fn assert_roundtrip_conversion(original_value: &value::Value, value_type: &schema::ValueType) { | ||
| Python::with_gil(|py| { | ||
| // Convert Rust value to Python object | ||
| let pythonized_value = Pythonized(original_value.clone()); |
There was a problem hiding this comment.
Actually I think I made a mistake in my last round of comments. Apologize for that..
There're actually two sets of conversion mechanism defined in this package:
- For conversion of some meta objects, e.g. configs (settings, specs, etc.),
Pythonizedis used. - For conversion of data values in CocoIndex,
value_to_py_object()andvalue_from_py_object()are used.
Really sorry for the confusion. Your previous code was actually correct in using value_from_py_object() and value_to_py_object().
With these, we don't need to add Deserialize for existing types.
There was a problem hiding this comment.
Ah. Thanks for clarification. I'll revert related changes. to use value_to_py_object and value_from_py_object
|
|
||
| println!("Roundtripped value: {:?}", roundtripped_value); | ||
| // Compare values | ||
| match (&original_value, &roundtripped_value) { |
There was a problem hiding this comment.
Since PartialEq is already provided for value types, I think we can directly compare these by == or assert_eq!?
There was a problem hiding this comment.
Will look into it. Did not realise that there is PartialEq already.
| @@ -1,13 +1,13 @@ | |||
| use serde::Deserialize; | |||
|
|
|||
| #[derive(Deserialize, Debug)] | |||
| #[derive(Deserialize, Debug, Clone)] | |||
There was a problem hiding this comment.
is Clone here needed by any new code?
fixes #438