On Mon, May 05, 2025 at 02:02:09PM +0800, David Gow wrote:
On Sat, 3 May 2025 at 05:51, Miguel Ojeda ojeda@kernel.org wrote:
Currently, return values of KUnit `#[test]` functions are ignored.
Thus introduce support for `-> Result` functions by checking their returned values.
At the same time, require that test functions return `()` or `Result<T, E>`, which should avoid mistakes, especially with non-`#[must_use]` types. Other types can be supported in the future if needed.
With this, a failing test like:
#[test] fn my_test() -> Result { f()?; Ok(()) }will output:
[ 3.744214] KTAP version 1 [ 3.744287] # Subtest: my_test_suite [ 3.744378] # speed: normal [ 3.744399] 1..1 [ 3.745817] # my_test: ASSERTION FAILED at rust/kernel/lib.rs:321 [ 3.745817] Expected is_test_result_ok(my_test()) to be true, but is false [ 3.747152] # my_test.speed: normal [ 3.747199] not ok 1 my_test [ 3.747345] not ok 4 my_test_suiteSigned-off-by: Miguel Ojeda ojeda@kernel.org
This is a neat idea. I think a lot of tests will still want to go with the () return value, but having both as an option seems like a good idea to me.
Handling the return value of a test is definitely a good thing, but I'm not sure returning `Err` should be treated as assertion failure though. Considering:
#[test] fn foo() -> Result { let b = KBox::new(...)?; // need to allocate memory for the test use_box(b); assert!(...); }
if the test runs without enough memory, then KBox::new() would return a Err(ENOMEM), and technically, it's not a test failure rather the test has been skipped because of no enough resource.
I would suggest we handle the `Err` returns specially (mark as "SKIPPED" maybe?).
Thoughts?
Regards, Boqun
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
rust/kernel/kunit.rs | 25 +++++++++++++++++++++++++ rust/macros/kunit.rs | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 2659895d4c5d..f43e3ed460c2 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -164,6 +164,31 @@ macro_rules! kunit_assert_eq { }}; }
+trait TestResult {
- fn is_test_result_ok(&self) -> bool;
+}
+impl TestResult for () {
- fn is_test_result_ok(&self) -> bool {
true- }
+}
+impl<T, E> TestResult for Result<T, E> {
- fn is_test_result_ok(&self) -> bool {
self.is_ok()- }
+}
+/// Returns whether a test result is to be considered OK. +/// +/// This will be `assert!`ed from the generated tests. +#[doc(hidden)] +#[expect(private_bounds)] +pub fn is_test_result_ok(t: impl TestResult) -> bool {
- t.is_test_result_ok()
+}
/// Represents an individual test case. /// /// The [`kunit_unsafe_test_suite!`] macro expects a NULL-terminated list of valid test cases. diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index eb4f2afdbe43..9681775d160a 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -105,8 +105,9 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { let path = crate::helpers::file(); for test in &tests { let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test);
// An extra `use` is used here to reduce the length of the message. let kunit_wrapper = format!(
"unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}",
"unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ use kernel::kunit::is_test_result_ok; assert!(is_test_result_ok({}())); }}", kunit_wrapper_fn_name, test ); writeln!(kunit_macros, "{kunit_wrapper}").unwrap();-- 2.49.0