Improve test coverage for src/call.rs#5951
Conversation
The check_call! macros in test_call and test_call_positional ignored their \ parameter, always calling with the outer args.clone() (Bound<'py, PyTuple>). All five invocations therefore tested only the Bound path, leaving &Bound, Py, &Py, and Borrowed impls uncovered. Fix both macros to use \ and \ so each invocation exercises the intended PyCallArgs impl. Add three new tests: - test_call_unit_args: covers ()::call and ()::call_positional impls - test_call_method_positional_default: covers the default call_method_positional trait method (happy path + AttributeError path) - test_call_error_paths: covers the error return paths in Borrowed::call and Borrowed::call_positional (TypeError from PyObject_Call)
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, I saw this was in draft, gave some suggestions anyway.
| check_call!(args.clone().unbind(), kwargs); | ||
|
|
||
| // &Py<PyTuple> | ||
| check_call!(&args.as_unbound(), kwargs); |
There was a problem hiding this comment.
Looks like this has uncovered another typo.
| check_call!(args.as_unbound(), kwargs); |
There was a problem hiding this comment.
It's not interesting to users if we add test coverage, can delete this file (I added label in CI).
| macro_rules! check_call { | ||
| ($args:expr, $kwargs:expr) => { |
There was a problem hiding this comment.
It looks like $kwargs is not used in this macro at all, should be removed?
| #[test] | ||
| fn test_call_unit_args() { | ||
| // Exercises ()::call and ()::call_positional — both routes through | ||
| // into_pyobject_or_pyerr to produce an empty PyTuple. | ||
| use crate::types::PyDict; | ||
| use crate::{ |
There was a problem hiding this comment.
This test possibly reveals something interesting - PyCallArgs<'py> for () can potentially use PyObject_VectorcallDict with empty array to avoid needing to pass the intermediate tuple unless the callee wants it.
| #[test] | ||
| fn test_call_method_positional_default() { | ||
| // Exercises the default call_method_positional impl on the PyCallArgs trait | ||
| // (lines 68–76 of call.rs): getattr + call1. Reached via call_method1. | ||
| use crate::{ | ||
| exceptions::PyAttributeError, | ||
| types::{PyAnyMethods, PyList, PyListMethods, PyTuple}, | ||
| Python, | ||
| }; | ||
|
|
||
| Python::attach(|py| { | ||
| let list = PyList::new(py, [1i32, 2, 3]).unwrap(); | ||
| let args = PyTuple::new(py, [4i32]).unwrap(); | ||
|
|
||
| // Happy path: getattr("append") succeeds, call1 appends the element. | ||
| list.call_method1("append", args).unwrap(); | ||
| assert_eq!(list.len(), 4); | ||
| // Verify the value is the integer 4, not a wrapped tuple. | ||
| assert_eq!(list.get_item(3).unwrap().extract::<i32>().unwrap(), 4i32); | ||
|
|
||
| // Error path: getattr fails → call_method_positional returns Err. | ||
| let err = list | ||
| .call_method1("nonexistent_method_xyz", PyTuple::empty(py)) | ||
| .unwrap_err(); | ||
| assert!(err.is_instance_of::<PyAttributeError>(py)); | ||
| }) | ||
| } |
There was a problem hiding this comment.
This reads like AI slop test which is heavily coupled to the current implementation (e.g. referencing implementation function names, line numbers). If there is merit in having such a test, I would rather it be rewritten to cover the possible error pathways presented by the API, not from the implementation.
| #[test] | ||
| fn test_call_error_paths() { | ||
| // Exercises the NULL-return error paths in Borrowed::call and | ||
| // Borrowed::call_positional when PyObject_Call fails. |
There was a problem hiding this comment.
Similar comment as above RE ai slop.
2 participants
Closes #2453 (partially). Fixes check_call! macro bug and adds tests for uncovered paths.