| Message ID | 20250502215133.1923676-3-ojeda@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rust KUnit `#[test]` support improvements | expand |
On Fri, May 2, 2025 at 5:53 PM 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. Why not restrict this to Result<(), E>? > > 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 Is it possible to include the error in the output? > [ 3.747152] # my_test.speed: normal > [ 3.747199] not ok 1 my_test > [ 3.747345] not ok 4 my_test_suite > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > 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 > >
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_suite > > Signed-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. 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 >
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_suite > > > > Signed-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 > >
On Tue, 6 May 2025 at 03:34, Boqun Feng <boqun.feng@gmail.com> wrote: > > 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_suite > > > > > > Signed-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 > FWIW, having out-of-memory situations trigger a test failure is consistent with what other KUnit tests (written in C) do. There's both advantages and disadvantages to this: on the one hand, it's prone to false positives (as you mention), on the other, it catches cases where the test is using an unusually large amount of memory (which could indeed be a test issues). My go-to rule is that tests should fail if small allocations (which, in the normal course of events, _should_ succeed) fail, but if they have unusual resource requirements (beyond "enough memory for the system to run normally") these should be checked separately when the test starts. That being said, I definitely think that, by default, an `Err` return should map to a FAILED test result, as not all Err Results are a resource exhaustion issue, and we definitely don't want to mark a test as skipped if there's a real error occurring. If test authors wish to skip a test when an out-of-memory condition occurs, they probably should handle that explicitly. (But I'd not be opposed to helpers to make it easier.) Cheers, -- David
On Tue, May 6, 2025 at 8:33 AM David Gow <davidgow@google.com> wrote: > > FWIW, having out-of-memory situations trigger a test failure is > consistent with what other KUnit tests (written in C) do. > > There's both advantages and disadvantages to this: on the one hand, > it's prone to false positives (as you mention), on the other, it > catches cases where the test is using an unusually large amount of > memory (which could indeed be a test issues). > > My go-to rule is that tests should fail if small allocations (which, > in the normal course of events, _should_ succeed) fail, but if they > have unusual resource requirements (beyond "enough memory for the > system to run normally") these should be checked separately when the > test starts. > > That being said, I definitely think that, by default, an `Err` return > should map to a FAILED test result, as not all Err Results are a > resource exhaustion issue, and we definitely don't want to mark a test > as skipped if there's a real error occurring. If test authors wish to > skip a test when an out-of-memory condition occurs, they probably > should handle that explicitly. (But I'd not be opposed to helpers to > make it easier.) Yeah, agreed. There may be value in differentiating errors coming from `?` vs. `assert!`s -- i.e. the discussion in the other thread. It could even be a different error state if that was valuable -- though I think over complicating is not great either. By the way, before I forget to write this down: I talked to Alice about this back when this discussion happened and she suggested having a `Result` that carries extra information -- the location. I was worried about the added cost of doing that in all cases (i.e. for every `Result` in the kernel), so I suggested doing that but opt-in, i.e. with a Kconfig option that would be in e.g. the debug menu. Then people could typically run their debug kernels and things like KUnit with it enabled, but production kernels without it. Then later on, if it proves useful for other things, we could try to figure out ways to do it without too much cost. Cheers, Miguel
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();
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_suite Signed-off-by: Miguel Ojeda <ojeda@kernel.org> --- rust/kernel/kunit.rs | 25 +++++++++++++++++++++++++ rust/macros/kunit.rs | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-)