Improvements that build on top of the very basic `#[test]` support merged in v6.15.
They are fairly minimal changes, but they allow us to map `assert*!`s back to KUnit, plus to add support for test functions that return `Result`s.
In essence, they get our `#[test]`s essentially on par with the documentation tests.
I also took the chance to convert some host `#[test]`s we had to KUnit in order to showcase the feature.
Finally, I added documentation that was lacking from the original submission.
I hope this helps.
Miguel Ojeda (7): rust: kunit: support KUnit-mapped `assert!` macros in `#[test]`s rust: kunit: support checked `-> Result`s in KUnit `#[test]`s rust: add `kunit_tests` to the prelude rust: str: convert `rusttest` tests into KUnit rust: str: take advantage of the `-> Result` support in KUnit `#[test]`'s Documentation: rust: rename `#[test]`s to "`rusttest` host tests" Documentation: rust: testing: add docs on the new KUnit `#[test]` tests
Documentation/rust/testing.rst | 80 ++++++++++++++++++++++++++++++++-- init/Kconfig | 3 ++ rust/Makefile | 3 +- rust/kernel/kunit.rs | 29 ++++++++++-- rust/kernel/prelude.rs | 2 +- rust/kernel/str.rs | 76 ++++++++++++++------------------ rust/macros/helpers.rs | 16 +++++++ rust/macros/kunit.rs | 31 ++++++++++++- rust/macros/lib.rs | 6 ++- 9 files changed, 191 insertions(+), 55 deletions(-)
base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e -- 2.49.0
The KUnit `#[test]` support that landed recently is very basic and does not map the `assert*!` macros into KUnit like the doctests do, so they panic at the moment.
Thus implement the custom mapping in a similar way to doctests, reusing the infrastructure there.
In Rust 1.88.0, the `file()` method in `Span` may be stable [1]. However, it was changed recently (from `SourceFile`), so we need to do something different in previous versions. Thus create a helper for it and use it to get the path.
With this, a failing test suite like:
#[kunit_tests(my_test_suite)] mod tests { use super::*;
#[test] fn my_first_test() { assert_eq!(42, 43); }
#[test] fn my_second_test() { assert!(42 >= 43); } }
will properly map back to KUnit, printing something like:
[ 1.924325] KTAP version 1 [ 1.924421] # Subtest: my_test_suite [ 1.924506] # speed: normal [ 1.924525] 1..2 [ 1.926385] # my_first_test: ASSERTION FAILED at rust/kernel/lib.rs:251 [ 1.926385] Expected 42 == 43 to be true, but is false [ 1.928026] # my_first_test.speed: normal [ 1.928075] not ok 1 my_first_test [ 1.928723] # my_second_test: ASSERTION FAILED at rust/kernel/lib.rs:256 [ 1.928723] Expected 42 >= 43 to be true, but is false [ 1.929834] # my_second_test.speed: normal [ 1.929868] not ok 2 my_second_test [ 1.930032] # my_test_suite: pass:0 fail:2 skip:0 total:2 [ 1.930153] # Totals: pass:0 fail:2 skip:0 total
Link: https://github.com/rust-lang/rust/pull/140514 [1] Signed-off-by: Miguel Ojeda ojeda@kernel.org --- init/Kconfig | 3 +++ rust/Makefile | 3 ++- rust/kernel/kunit.rs | 1 - rust/macros/helpers.rs | 16 ++++++++++++++++ rust/macros/kunit.rs | 28 +++++++++++++++++++++++++++- rust/macros/lib.rs | 4 ++++ 6 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig index 63f5974b9fa6..5f442c64c47b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -140,6 +140,9 @@ config LD_CAN_USE_KEEP_IN_OVERLAY config RUSTC_HAS_COERCE_POINTEE def_bool RUSTC_VERSION >= 108400
+config RUSTC_HAS_SPAN_FILE + def_bool RUSTC_VERSION >= 108800 + config PAHOLE_VERSION int default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) diff --git a/rust/Makefile b/rust/Makefile index 3aca903a7d08..075b38a24997 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -402,7 +402,8 @@ quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@ -Clink-args='$(call escsq,$(KBUILD_PROCMACROLDFLAGS))' \ --emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \ --crate-type proc-macro \ - --crate-name $(patsubst lib%.$(libmacros_extension),%,$(notdir $@)) $< + --crate-name $(patsubst lib%.$(libmacros_extension),%,$(notdir $@)) \ + @$(objtree)/include/generated/rustc_cfg $<
# Procedural macros can only be used with the `rustc` that compiled it. $(obj)/$(libmacros_name): $(src)/macros/lib.rs FORCE diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 1604fb6a5b1b..2659895d4c5d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -323,7 +323,6 @@ mod tests {
#[test] fn rust_test_kunit_example_test() { - #![expect(clippy::eq_op)] assert_eq!(1 + 1, 2); }
diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs index a3ee27e29a6f..57c3b0f0c194 100644 --- a/rust/macros/helpers.rs +++ b/rust/macros/helpers.rs @@ -86,3 +86,19 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> { } None } + +pub(crate) fn file() -> String { + #[cfg(not(CONFIG_RUSTC_HAS_SPAN_FILE))] + { + proc_macro::Span::call_site() + .source_file() + .path() + .to_string_lossy() + .into_owned() + } + + #[cfg(CONFIG_RUSTC_HAS_SPAN_FILE)] + { + proc_macro::Span::call_site().file() + } +} diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 4f553ecf40c0..eb4f2afdbe43 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -101,6 +101,8 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { // ``` let mut kunit_macros = "".to_owned(); let mut test_cases = "".to_owned(); + let mut assert_macros = "".to_owned(); + let path = crate::helpers::file(); for test in &tests { let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test); let kunit_wrapper = format!( @@ -114,6 +116,27 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { test, kunit_wrapper_fn_name ) .unwrap(); + writeln!( + assert_macros, + r#" +/// Overrides the usual [`assert!`] macro with one that calls KUnit instead. +#[allow(unused)] +macro_rules! assert {{ + ($cond:expr $(,)?) => {{{{ + kernel::kunit_assert!("{test}", "{path}", 0, $cond); + }}}} +}} + +/// Overrides the usual [`assert_eq!`] macro with one that calls KUnit instead. +#[allow(unused)] +macro_rules! assert_eq {{ + ($left:expr, $right:expr $(,)?) => {{{{ + kernel::kunit_assert_eq!("{test}", "{path}", 0, $left, $right); + }}}} +}} + "# + ) + .unwrap(); }
writeln!(kunit_macros).unwrap(); @@ -152,7 +175,10 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { } }
- let mut new_body = TokenStream::from_iter(new_body); + let body = new_body; + let mut new_body = TokenStream::new(); + new_body.extend::<TokenStream>(assert_macros.parse().unwrap()); + new_body.extend(body); new_body.extend::<TokenStream>(kunit_macros.parse().unwrap());
tokens.push(TokenTree::Group(Group::new(Delimiter::Brace, new_body))); diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 9acaa68c974e..8bd7906276be 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -6,6 +6,10 @@ // and thus add a dependency on `include/config/RUSTC_VERSION_TEXT`, which is // touched by Kconfig when the version string from the compiler changes.
+// TODO: check that when Rust 1.88.0 is released, this would be enough: +// #![cfg_attr(not(CONFIG_RUSTC_HAS_SPAN_FILE), feature(proc_macro_span))] +#![feature(proc_macro_span)] + #[macro_use] mod quote; mod concat_idents;
On Fri, May 2, 2025 at 5:53 PM Miguel Ojeda ojeda@kernel.org wrote:
The KUnit `#[test]` support that landed recently is very basic and does not map the `assert*!` macros into KUnit like the doctests do, so they panic at the moment.
Thus implement the custom mapping in a similar way to doctests, reusing the infrastructure there.
In Rust 1.88.0, the `file()` method in `Span` may be stable [1]. However, it was changed recently (from `SourceFile`), so we need to do something different in previous versions. Thus create a helper for it and use it to get the path.
With this, a failing test suite like:
#[kunit_tests(my_test_suite)] mod tests { use super::*; #[test] fn my_first_test() { assert_eq!(42, 43); } #[test] fn my_second_test() { assert!(42 >= 43); } }will properly map back to KUnit, printing something like:
[ 1.924325] KTAP version 1 [ 1.924421] # Subtest: my_test_suite [ 1.924506] # speed: normal [ 1.924525] 1..2 [ 1.926385] # my_first_test: ASSERTION FAILED at rust/kernel/lib.rs:251 [ 1.926385] Expected 42 == 43 to be true, but is false [ 1.928026] # my_first_test.speed: normal [ 1.928075] not ok 1 my_first_test [ 1.928723] # my_second_test: ASSERTION FAILED at rust/kernel/lib.rs:256 [ 1.928723] Expected 42 >= 43 to be true, but is false [ 1.929834] # my_second_test.speed: normal [ 1.929868] not ok 2 my_second_test [ 1.930032] # my_test_suite: pass:0 fail:2 skip:0 total:2 [ 1.930153] # Totals: pass:0 fail:2 skip:0 totalLink: https://github.com/rust-lang/rust/pull/140514 [1] Signed-off-by: Miguel Ojeda ojeda@kernel.org
init/Kconfig | 3 +++ rust/Makefile | 3 ++- rust/kernel/kunit.rs | 1 - rust/macros/helpers.rs | 16 ++++++++++++++++ rust/macros/kunit.rs | 28 +++++++++++++++++++++++++++- rust/macros/lib.rs | 4 ++++ 6 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig index 63f5974b9fa6..5f442c64c47b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -140,6 +140,9 @@ config LD_CAN_USE_KEEP_IN_OVERLAY config RUSTC_HAS_COERCE_POINTEE def_bool RUSTC_VERSION >= 108400
+config RUSTC_HAS_SPAN_FILE
def_bool RUSTC_VERSION >= 108800config PAHOLE_VERSION int default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) diff --git a/rust/Makefile b/rust/Makefile index 3aca903a7d08..075b38a24997 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -402,7 +402,8 @@ quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@ -Clink-args='$(call escsq,$(KBUILD_PROCMACROLDFLAGS))' \ --emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \ --crate-type proc-macro \
--crate-name $(patsubst lib%.$(libmacros_extension),%,$(notdir $@)) $<
--crate-name $(patsubst lib%.$(libmacros_extension),%,$(notdir $@)) \@$(objtree)/include/generated/rustc_cfg $<# Procedural macros can only be used with the `rustc` that compiled it. $(obj)/$(libmacros_name): $(src)/macros/lib.rs FORCE diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 1604fb6a5b1b..2659895d4c5d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -323,7 +323,6 @@ mod tests {
#[test] fn rust_test_kunit_example_test() {
#![expect(clippy::eq_op)]
How come this vanished?
assert_eq!(1 + 1, 2); }diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs index a3ee27e29a6f..57c3b0f0c194 100644 --- a/rust/macros/helpers.rs +++ b/rust/macros/helpers.rs @@ -86,3 +86,19 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> { } None }
+pub(crate) fn file() -> String {
- #[cfg(not(CONFIG_RUSTC_HAS_SPAN_FILE))]
- {
proc_macro::Span::call_site().source_file().path().to_string_lossy().into_owned()- }
- #[cfg(CONFIG_RUSTC_HAS_SPAN_FILE)]
- {
proc_macro::Span::call_site().file()- }
+} diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 4f553ecf40c0..eb4f2afdbe43 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -101,6 +101,8 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { // ``` let mut kunit_macros = "".to_owned(); let mut test_cases = "".to_owned();
- let mut assert_macros = "".to_owned();
nit: why not String::new() for all these?
- let path = crate::helpers::file(); for test in &tests { let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test); let kunit_wrapper = format!(
@@ -114,6 +116,27 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { test, kunit_wrapper_fn_name ) .unwrap();
writeln!(assert_macros,r#"+/// Overrides the usual [`assert!`] macro with one that calls KUnit instead. +#[allow(unused)] +macro_rules! assert {{
- ($cond:expr $(,)?) => {{{{
kernel::kunit_assert!("{test}", "{path}", 0, $cond);- }}}}
+}}
+/// Overrides the usual [`assert_eq!`] macro with one that calls KUnit instead. +#[allow(unused)] +macro_rules! assert_eq {{
- ($left:expr, $right:expr $(,)?) => {{{{
kernel::kunit_assert_eq!("{test}", "{path}", 0, $left, $right);- }}}}
+}}
"#).unwrap();}
writeln!(kunit_macros).unwrap();
@@ -152,7 +175,10 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { } }
- let mut new_body = TokenStream::from_iter(new_body);
- let body = new_body;
- let mut new_body = TokenStream::new();
- new_body.extend::<TokenStream>(assert_macros.parse().unwrap());
- new_body.extend(body);
Could we do this (pushing `assert_macros`) before the block above to avoid this body/new_body name juggling?
new_body.extend::<TokenStream>(kunit_macros.parse().unwrap()); tokens.push(TokenTree::Group(Group::new(Delimiter::Brace, new_body)));diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 9acaa68c974e..8bd7906276be 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -6,6 +6,10 @@ // and thus add a dependency on `include/config/RUSTC_VERSION_TEXT`, which is // touched by Kconfig when the version string from the compiler changes.
+// TODO: check that when Rust 1.88.0 is released, this would be enough: +// #![cfg_attr(not(CONFIG_RUSTC_HAS_SPAN_FILE), feature(proc_macro_span))] +#![feature(proc_macro_span)]
#[macro_use] mod quote; mod concat_idents; -- 2.49.0
On Sat, 3 May 2025 at 05:51, Miguel Ojeda ojeda@kernel.org wrote:
The KUnit `#[test]` support that landed recently is very basic and does not map the `assert*!` macros into KUnit like the doctests do, so they panic at the moment.
Thus implement the custom mapping in a similar way to doctests, reusing the infrastructure there.
In Rust 1.88.0, the `file()` method in `Span` may be stable [1]. However, it was changed recently (from `SourceFile`), so we need to do something different in previous versions. Thus create a helper for it and use it to get the path.
With this, a failing test suite like:
#[kunit_tests(my_test_suite)] mod tests { use super::*; #[test] fn my_first_test() { assert_eq!(42, 43); } #[test] fn my_second_test() { assert!(42 >= 43); } }will properly map back to KUnit, printing something like:
[ 1.924325] KTAP version 1 [ 1.924421] # Subtest: my_test_suite [ 1.924506] # speed: normal [ 1.924525] 1..2 [ 1.926385] # my_first_test: ASSERTION FAILED at rust/kernel/lib.rs:251 [ 1.926385] Expected 42 == 43 to be true, but is false [ 1.928026] # my_first_test.speed: normal [ 1.928075] not ok 1 my_first_test [ 1.928723] # my_second_test: ASSERTION FAILED at rust/kernel/lib.rs:256 [ 1.928723] Expected 42 >= 43 to be true, but is false [ 1.929834] # my_second_test.speed: normal [ 1.929868] not ok 2 my_second_test [ 1.930032] # my_test_suite: pass:0 fail:2 skip:0 total:2 [ 1.930153] # Totals: pass:0 fail:2 skip:0 totalLink: https://github.com/rust-lang/rust/pull/140514 [1] Signed-off-by: Miguel Ojeda ojeda@kernel.org
Nice! While I do think there may still be some cases where we might want to use KUnit-specific macros in the future (particularly if we have more complex, multithreaded test contexts), this is definitely better for most cases.
I also managed to test it against the 1.88 nightly, and the message is looking good.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
init/Kconfig | 3 +++ rust/Makefile | 3 ++- rust/kernel/kunit.rs | 1 - rust/macros/helpers.rs | 16 ++++++++++++++++ rust/macros/kunit.rs | 28 +++++++++++++++++++++++++++- rust/macros/lib.rs | 4 ++++ 6 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig index 63f5974b9fa6..5f442c64c47b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -140,6 +140,9 @@ config LD_CAN_USE_KEEP_IN_OVERLAY config RUSTC_HAS_COERCE_POINTEE def_bool RUSTC_VERSION >= 108400
+config RUSTC_HAS_SPAN_FILE
def_bool RUSTC_VERSION >= 108800config PAHOLE_VERSION int default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) diff --git a/rust/Makefile b/rust/Makefile index 3aca903a7d08..075b38a24997 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -402,7 +402,8 @@ quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@ -Clink-args='$(call escsq,$(KBUILD_PROCMACROLDFLAGS))' \ --emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \ --crate-type proc-macro \
--crate-name $(patsubst lib%.$(libmacros_extension),%,$(notdir $@)) $<
--crate-name $(patsubst lib%.$(libmacros_extension),%,$(notdir $@)) \@$(objtree)/include/generated/rustc_cfg $<# Procedural macros can only be used with the `rustc` that compiled it. $(obj)/$(libmacros_name): $(src)/macros/lib.rs FORCE diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 1604fb6a5b1b..2659895d4c5d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -323,7 +323,6 @@ mod tests {
#[test] fn rust_test_kunit_example_test() {
#![expect(clippy::eq_op)] assert_eq!(1 + 1, 2);diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs index a3ee27e29a6f..57c3b0f0c194 100644 --- a/rust/macros/helpers.rs +++ b/rust/macros/helpers.rs @@ -86,3 +86,19 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> { } None }
+pub(crate) fn file() -> String {
- #[cfg(not(CONFIG_RUSTC_HAS_SPAN_FILE))]
- {
proc_macro::Span::call_site().source_file().path().to_string_lossy().into_owned()- }
- #[cfg(CONFIG_RUSTC_HAS_SPAN_FILE)]
- {
proc_macro::Span::call_site().file()- }
+} diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 4f553ecf40c0..eb4f2afdbe43 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -101,6 +101,8 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { // ``` let mut kunit_macros = "".to_owned(); let mut test_cases = "".to_owned();
- let mut assert_macros = "".to_owned();
- let path = crate::helpers::file(); for test in &tests { let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test); let kunit_wrapper = format!(
@@ -114,6 +116,27 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { test, kunit_wrapper_fn_name ) .unwrap();
writeln!(assert_macros,r#"+/// Overrides the usual [`assert!`] macro with one that calls KUnit instead. +#[allow(unused)] +macro_rules! assert {{
- ($cond:expr $(,)?) => {{{{
kernel::kunit_assert!("{test}", "{path}", 0, $cond);- }}}}
+}}
+/// Overrides the usual [`assert_eq!`] macro with one that calls KUnit instead. +#[allow(unused)] +macro_rules! assert_eq {{
- ($left:expr, $right:expr $(,)?) => {{{{
kernel::kunit_assert_eq!("{test}", "{path}", 0, $left, $right);- }}}}
+}}
"#).unwrap();}
writeln!(kunit_macros).unwrap();
@@ -152,7 +175,10 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { } }
- let mut new_body = TokenStream::from_iter(new_body);
let body = new_body;
let mut new_body = TokenStream::new();
new_body.extend::<TokenStream>(assert_macros.parse().unwrap());
new_body.extend(body); new_body.extend::<TokenStream>(kunit_macros.parse().unwrap());
tokens.push(TokenTree::Group(Group::new(Delimiter::Brace, new_body)));
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 9acaa68c974e..8bd7906276be 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -6,6 +6,10 @@ // and thus add a dependency on `include/config/RUSTC_VERSION_TEXT`, which is // touched by Kconfig when the version string from the compiler changes.
+// TODO: check that when Rust 1.88.0 is released, this would be enough: +// #![cfg_attr(not(CONFIG_RUSTC_HAS_SPAN_FILE), feature(proc_macro_span))] +#![feature(proc_macro_span)]
#[macro_use] mod quote; mod concat_idents; -- 2.49.0
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(-)
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();
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_suiteSigned-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 Sun, May 4, 2025 at 7:34 PM Tamir Duberstein tamird@gmail.com wrote:
Why not restrict this to Result<(), E>?
I guess it is an option -- not sure if there may be a use case.
Is it possible to include the error in the output?
I thought about giving some more context somehow and perhaps printing it "manually" in the log, possibly in a KUnit `# ...`. David can probably suggest the "proper" way.
Thanks!
Cheers, Miguel
On Mon, 5 May 2025 at 02:00, Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, May 4, 2025 at 7:34 PM Tamir Duberstein tamird@gmail.com wrote:
Why not restrict this to Result<(), E>?
I guess it is an option -- not sure if there may be a use case.
Is it possible to include the error in the output?
I thought about giving some more context somehow and perhaps printing it "manually" in the log, possibly in a KUnit `# ...`. David can probably suggest the "proper" way.
Yeah, writing it to the log is fine: probably the best way of handling this would be to have a kunit assertion macro for "assert_is_ok!()", which prints the error nicely. We could just use the existing kernel::kunit::err() function (which could use some improvement, but is a good start), or even add a proper assertion formatter which handles rust types via %pA.
That being said, there's no guarantee that the Err branch of a Result<> can be printed: so there'd need to be some magic to handle both the case where (e.g.) Err derives Debug, and the case where it doesn't (in which case, we're basically stuck with what we've got here: "expected is_ok()" or similar.
Cheers, -- David
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.
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_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
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_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
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
It is convenient to have certain things in the `kernel` prelude, and means kernel developers will find it even easier to start writing tests.
And, anyway, nobody should need to use this identifier for anything else.
Thus add it to the prelude.
Signed-off-by: Miguel Ojeda ojeda@kernel.org --- rust/kernel/kunit.rs | 3 +-- rust/kernel/prelude.rs | 2 +- rust/macros/lib.rs | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index f43e3ed460c2..053a7da147d5 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -6,6 +6,7 @@ //! //! Reference: https://docs.kernel.org/dev-tools/kunit/index.html
+use crate::prelude::*; use core::{ffi::c_void, fmt};
/// Prints a KUnit error-level message. @@ -40,8 +41,6 @@ pub fn info(args: fmt::Arguments<'_>) { } }
-use macros::kunit_tests; - /// Asserts that a boolean expression is `true` at runtime. /// /// Public but hidden since it should only be used from generated tests. diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs index baa774a351ce..e5d61a83952f 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -17,7 +17,7 @@ pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
#[doc(no_inline)] -pub use macros::{export, module, vtable}; +pub use macros::{export, kunit_tests, module, vtable};
pub use pin_init::{init, pin_data, pin_init, pinned_drop, InPlaceWrite, Init, PinInit, Zeroable};
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 8bd7906276be..8b8d46e759d4 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -406,7 +406,7 @@ pub fn paste(input: TokenStream) -> TokenStream { /// # Examples /// /// ```ignore -/// # use macros::kunit_tests; +/// # use kernel::prelude::*; /// #[kunit_tests(kunit_test_suit_name)] /// mod tests { /// #[test]
On Sat, 3 May 2025 at 05:52, Miguel Ojeda ojeda@kernel.org wrote:
It is convenient to have certain things in the `kernel` prelude, and means kernel developers will find it even easier to start writing tests.
And, anyway, nobody should need to use this identifier for anything else.
Thus add it to the prelude.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
Definitely a fan of this here, thanks!
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
rust/kernel/kunit.rs | 3 +-- rust/kernel/prelude.rs | 2 +- rust/macros/lib.rs | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index f43e3ed460c2..053a7da147d5 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -6,6 +6,7 @@ //! //! Reference: https://docs.kernel.org/dev-tools/kunit/index.html
+use crate::prelude::*; use core::{ffi::c_void, fmt};
/// Prints a KUnit error-level message. @@ -40,8 +41,6 @@ pub fn info(args: fmt::Arguments<'_>) { } }
-use macros::kunit_tests;
/// Asserts that a boolean expression is `true` at runtime. /// /// Public but hidden since it should only be used from generated tests. diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs index baa774a351ce..e5d61a83952f 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -17,7 +17,7 @@ pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
#[doc(no_inline)] -pub use macros::{export, module, vtable}; +pub use macros::{export, kunit_tests, module, vtable};
pub use pin_init::{init, pin_data, pin_init, pinned_drop, InPlaceWrite, Init, PinInit, Zeroable};
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 8bd7906276be..8b8d46e759d4 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -406,7 +406,7 @@ pub fn paste(input: TokenStream) -> TokenStream { /// # Examples /// /// ```ignore -/// # use macros::kunit_tests; +/// # use kernel::prelude::*; /// #[kunit_tests(kunit_test_suit_name)] /// mod tests { /// #[test] -- 2.49.0
In general, we should aim to test as much as possible within the actual kernel, and not in the build host.
Thus convert these `rusttest` tests into KUnit tests.
Signed-off-by: Miguel Ojeda ojeda@kernel.org --- rust/kernel/str.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 878111cb77bc..cf2caa2db168 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -6,7 +6,7 @@ use core::fmt::{self, Write}; use core::ops::{self, Deref, DerefMut, Index};
-use crate::error::{code::*, Error}; +use crate::prelude::*;
/// Byte string without UTF-8 validity guarantee. #[repr(transparent)] @@ -572,8 +572,7 @@ macro_rules! c_str { }}; }
-#[cfg(test)] -#[expect(clippy::items_after_test_module)] +#[kunit_tests(rust_kernel_str)] mod tests { use super::*;
@@ -622,11 +621,10 @@ fn test_cstr_to_str() { }
#[test] - #[should_panic] - fn test_cstr_to_str_panic() { + fn test_cstr_to_str_invalid_utf8() { let bad_bytes = b"\xc3\x28\0"; let checked_cstr = CStr::from_bytes_with_nul(bad_bytes).unwrap(); - checked_cstr.to_str().unwrap(); + assert!(checked_cstr.to_str().is_err()); }
#[test]
On Fri, May 2, 2025 at 5:53 PM Miguel Ojeda ojeda@kernel.org wrote:
In general, we should aim to test as much as possible within the actual kernel, and not in the build host.
Is that true? The build host is often easier to work with. There's a number of host tests on the C side that exist precisely for this reason.
Thus convert these `rusttest` tests into KUnit tests.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
rust/kernel/str.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 878111cb77bc..cf2caa2db168 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -6,7 +6,7 @@ use core::fmt::{self, Write}; use core::ops::{self, Deref, DerefMut, Index};
-use crate::error::{code::*, Error}; +use crate::prelude::*;
/// Byte string without UTF-8 validity guarantee. #[repr(transparent)] @@ -572,8 +572,7 @@ macro_rules! c_str { }}; }
-#[cfg(test)] -#[expect(clippy::items_after_test_module)] +#[kunit_tests(rust_kernel_str)] mod tests { use super::*;
@@ -622,11 +621,10 @@ fn test_cstr_to_str() { }
#[test]
- #[should_panic]
- fn test_cstr_to_str_panic() {
- fn test_cstr_to_str_invalid_utf8() { let bad_bytes = b"\xc3\x28\0"; let checked_cstr = CStr::from_bytes_with_nul(bad_bytes).unwrap();
checked_cstr.to_str().unwrap();
assert!(checked_cstr.to_str().is_err());}
#[test]
-- 2.49.0
On Sun, May 4, 2025 at 7:31 PM Tamir Duberstein tamird@gmail.com wrote:
Is that true? The build host is often easier to work with. There's a number of host tests on the C side that exist precisely for this reason.
Even for tests that could run in the host (pure functions), if you test in the host, then you are not testing the actual kernel code, in the sense of same compile flags, target, etc.
Moreover, you have UML, which gives you access to other APIs.
As for "easier to work with", I am not sure what you mean -- KUnit does not really require anything special w.r.t. building the kernel normally. In a way, these restricted host tests actually are an extra hassle, in that you have to deal with yet another test environment and special restrictions.
But which host tests are you referring to?
Thanks for reviewing!
Cheers, Miguel
On Sun, May 4, 2025 at 2:31 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, May 4, 2025 at 7:31 PM Tamir Duberstein tamird@gmail.com wrote:
Is that true? The build host is often easier to work with. There's a number of host tests on the C side that exist precisely for this reason.
Even for tests that could run in the host (pure functions), if you test in the host, then you are not testing the actual kernel code, in the sense of same compile flags, target, etc.
Moreover, you have UML, which gives you access to other APIs.
As for "easier to work with", I am not sure what you mean -- KUnit does not really require anything special w.r.t. building the kernel normally. In a way, these restricted host tests actually are an extra hassle, in that you have to deal with yet another test environment and special restrictions.
All good points.
But which host tests are you referring to?
One example is https://github.com/torvalds/linux/blob/59c9ab3e8cc7f56cd65608f6e938b5ae96eb9....
It might be that these are necessary because the xarray tests don't use kunit, and so are pretty inconvenient to run. As you might have guessed, I discovered these host tests when my patch porting the xarray tests to kunit broke the host-side build :(
On Sun, May 4, 2025 at 8:39 PM Tamir Duberstein tamird@gmail.com wrote:
One example is https://github.com/torvalds/linux/blob/59c9ab3e8cc7f56cd65608f6e938b5ae96eb9....
It might be that these are necessary because the xarray tests don't use kunit, and so are pretty inconvenient to run. As you might have guessed, I discovered these host tests when my patch porting the xarray tests to kunit broke the host-side build :(
It can be useful to have some tests as independent userspace things (i.e. outside KUnit-UML) to use other tooling on it, but I think for such cases we would want to have a way to use the tests from userspace without having to remove them from being KUnit tests too, since we definitely want to test them in the actual kernel too.
David et al. can probably tell us more context, e.g. I may be missing some plans on their side here. For instance, for Rust, we wanted to eventually have a way to tag stuff as kernel vs. host etc., but that is longer term.
Cheers, Miguel
On Mon, 5 May 2025 at 03:02, Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, May 4, 2025 at 8:39 PM Tamir Duberstein tamird@gmail.com wrote:
One example is https://github.com/torvalds/linux/blob/59c9ab3e8cc7f56cd65608f6e938b5ae96eb9....
It might be that these are necessary because the xarray tests don't use kunit, and so are pretty inconvenient to run. As you might have guessed, I discovered these host tests when my patch porting the xarray tests to kunit broke the host-side build :(
It can be useful to have some tests as independent userspace things (i.e. outside KUnit-UML) to use other tooling on it, but I think for such cases we would want to have a way to use the tests from userspace without having to remove them from being KUnit tests too, since we definitely want to test them in the actual kernel too.
David et al. can probably tell us more context, e.g. I may be missing some plans on their side here. For instance, for Rust, we wanted to eventually have a way to tag stuff as kernel vs. host etc., but that is longer term.
Yeah, this ultimately boils down to a combination of which tradeoffs are best for a given test, and personal preference.
KUnit definitely has the advantage of being more a more "accurate" test in a kernel environment — particularly if you're cross-compiling — but is a bit slower and more bloated (due to having the whole kernel), and therefore a bit more difficult to debug.
In an ideal world, most tests would be able to be compiled either as a host-side, standalone test or as a KUnit test. There are some (sadly lower-priority) plans to support this more with C code by having a standalone implementation of the KUnit API, and things like the automatic conversion of, e.g., assert!() macros into their KUnit equivalent could help if we wanted to try something similar on the Rust side.
But, as you point out, that sort of tagging of tests is really a longer-term goal.
In the meantime, my strategy has been to encourage KUnit for new tests, or ports where it's not going to break existing workflows too much, but ultimately to defer to the maintainer of the tests / code being tested if they've got strong opinions. (Of course, I am biased in KUnit's favour. :-))
-- David
On Sat, 3 May 2025 at 05:52, Miguel Ojeda ojeda@kernel.org wrote:
In general, we should aim to test as much as possible within the actual kernel, and not in the build host.
Thus convert these `rusttest` tests into KUnit tests.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
I'm obviously a fan of this. Assuming no-one working on the str code strenuously objects, let's do it!
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
rust/kernel/str.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 878111cb77bc..cf2caa2db168 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -6,7 +6,7 @@ use core::fmt::{self, Write}; use core::ops::{self, Deref, DerefMut, Index};
-use crate::error::{code::*, Error}; +use crate::prelude::*;
/// Byte string without UTF-8 validity guarantee. #[repr(transparent)] @@ -572,8 +572,7 @@ macro_rules! c_str { }}; }
-#[cfg(test)] -#[expect(clippy::items_after_test_module)] +#[kunit_tests(rust_kernel_str)] mod tests { use super::*;
@@ -622,11 +621,10 @@ fn test_cstr_to_str() { }
#[test]
- #[should_panic]
- fn test_cstr_to_str_panic() {
- fn test_cstr_to_str_invalid_utf8() { let bad_bytes = b"\xc3\x28\0"; let checked_cstr = CStr::from_bytes_with_nul(bad_bytes).unwrap();
checked_cstr.to_str().unwrap();
assert!(checked_cstr.to_str().is_err());}
#[test]
-- 2.49.0
On 5/2/25 2:51 PM, Miguel Ojeda wrote:
In general, we should aim to test as much as possible within the actual kernel, and not in the build host.
Thus convert these `rusttest` tests into KUnit tests.
yes yes yes! :)
Like many, many kernel developers, I've been using separate development and test machines, and so "make test" approaches are broken by design.
In other works, launching tests from make(1) usually includes the fatal flaw of leaving all the dependencies connected. And so when you try to run on your test machine, it tries to rebuild, but the kernel was build on the development machine...
It's just a constraint that should not be imposed on developers.
thanks,
Since now we have support for returning `-> Result`s, we can convert some of these tests to use the feature, and serve as a first user for it too.
Thus convert them.
This, in turn, simplifies them a fair bit.
We keep the actual assertions we want to make as explicit ones with `assert*!`s.
Signed-off-by: Miguel Ojeda ojeda@kernel.org --- rust/kernel/str.rs | 68 ++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 38 deletions(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index cf2caa2db168..8dcfb11013f2 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -576,25 +576,9 @@ macro_rules! c_str { mod tests { use super::*;
- struct String(CString); - - impl String { - fn from_fmt(args: fmt::Arguments<'_>) -> Self { - String(CString::try_from_fmt(args).unwrap()) - } - } - - impl Deref for String { - type Target = str; - - fn deref(&self) -> &str { - self.0.to_str().unwrap() - } - } - macro_rules! format { ($($f:tt)*) => ({ - &*String::from_fmt(kernel::fmt!($($f)*)) + CString::try_from_fmt(kernel::fmt!($($f)*))?.to_str()? }) }
@@ -613,66 +597,72 @@ macro_rules! format { \xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff";
#[test] - fn test_cstr_to_str() { + fn test_cstr_to_str() -> Result { let good_bytes = b"\xf0\x9f\xa6\x80\0"; - let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap(); - let checked_str = checked_cstr.to_str().unwrap(); + let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?; + let checked_str = checked_cstr.to_str()?; assert_eq!(checked_str, "🦀"); + Ok(()) }
#[test] - fn test_cstr_to_str_invalid_utf8() { + fn test_cstr_to_str_invalid_utf8() -> Result { let bad_bytes = b"\xc3\x28\0"; - let checked_cstr = CStr::from_bytes_with_nul(bad_bytes).unwrap(); + let checked_cstr = CStr::from_bytes_with_nul(bad_bytes)?; assert!(checked_cstr.to_str().is_err()); + Ok(()) }
#[test] - fn test_cstr_as_str_unchecked() { + fn test_cstr_as_str_unchecked() -> Result { let good_bytes = b"\xf0\x9f\x90\xA7\0"; - let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap(); + let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?; // SAFETY: The contents come from a string literal which contains valid UTF-8. let unchecked_str = unsafe { checked_cstr.as_str_unchecked() }; assert_eq!(unchecked_str, "🐧"); + Ok(()) }
#[test] - fn test_cstr_display() { - let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap(); + fn test_cstr_display() -> Result { + let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; assert_eq!(format!("{}", hello_world), "hello, world!"); - let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0").unwrap(); + let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; assert_eq!(format!("{}", non_printables), "\x01\x09\x0a"); - let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0").unwrap(); + let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; assert_eq!(format!("{}", non_ascii), "d\xe9j\xe0 vu"); - let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0").unwrap(); + let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; assert_eq!(format!("{}", good_bytes), "\xf0\x9f\xa6\x80"); + Ok(()) }
#[test] - fn test_cstr_display_all_bytes() { + fn test_cstr_display_all_bytes() -> Result { let mut bytes: [u8; 256] = [0; 256]; // fill `bytes` with [1..=255] + [0] for i in u8::MIN..=u8::MAX { bytes[i as usize] = i.wrapping_add(1); } - let cstr = CStr::from_bytes_with_nul(&bytes).unwrap(); + let cstr = CStr::from_bytes_with_nul(&bytes)?; assert_eq!(format!("{}", cstr), ALL_ASCII_CHARS); + Ok(()) }
#[test] - fn test_cstr_debug() { - let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap(); + fn test_cstr_debug() -> Result { + let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; assert_eq!(format!("{:?}", hello_world), ""hello, world!""); - let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0").unwrap(); + let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; assert_eq!(format!("{:?}", non_printables), ""\x01\x09\x0a""); - let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0").unwrap(); + let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; assert_eq!(format!("{:?}", non_ascii), ""d\xe9j\xe0 vu""); - let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0").unwrap(); + let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; assert_eq!(format!("{:?}", good_bytes), ""\xf0\x9f\xa6\x80""); + Ok(()) }
#[test] - fn test_bstr_display() { + fn test_bstr_display() -> Result { let hello_world = BStr::from_bytes(b"hello, world!"); assert_eq!(format!("{}", hello_world), "hello, world!"); let escapes = BStr::from_bytes(b"_\t_\n_\r_\_'_"_"); @@ -683,10 +673,11 @@ fn test_bstr_display() { assert_eq!(format!("{}", non_ascii), "d\xe9j\xe0 vu"); let good_bytes = BStr::from_bytes(b"\xf0\x9f\xa6\x80"); assert_eq!(format!("{}", good_bytes), "\xf0\x9f\xa6\x80"); + Ok(()) }
#[test] - fn test_bstr_debug() { + fn test_bstr_debug() -> Result { let hello_world = BStr::from_bytes(b"hello, world!"); assert_eq!(format!("{:?}", hello_world), ""hello, world!""); let escapes = BStr::from_bytes(b"_\t_\n_\r_\_'_"_"); @@ -697,6 +688,7 @@ fn test_bstr_debug() { assert_eq!(format!("{:?}", non_ascii), ""d\xe9j\xe0 vu""); let good_bytes = BStr::from_bytes(b"\xf0\x9f\xa6\x80"); assert_eq!(format!("{:?}", good_bytes), ""\xf0\x9f\xa6\x80""); + Ok(()) } }
On Fri, May 2, 2025 at 5:54 PM Miguel Ojeda ojeda@kernel.org wrote:
Since now we have support for returning `-> Result`s, we can convert some of these tests to use the feature, and serve as a first user for it too.
Thus convert them.
This, in turn, simplifies them a fair bit.
We keep the actual assertions we want to make as explicit ones with `assert*!`s.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
Alice pointed this out in another thread but: one of the downsides of returning Result is that in the event of failure the line number where the error occurred is no longer contained in the test output. I'm 👎 on this change for that reason.
rust/kernel/str.rs | 68 ++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 38 deletions(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index cf2caa2db168..8dcfb11013f2 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -576,25 +576,9 @@ macro_rules! c_str { mod tests { use super::*;
- struct String(CString);
- impl String {
fn from_fmt(args: fmt::Arguments<'_>) -> Self {String(CString::try_from_fmt(args).unwrap())}- }
- impl Deref for String {
type Target = str;fn deref(&self) -> &str {self.0.to_str().unwrap()}- }
These changes don't depend on returning `Result` from the tests AFAICT. Can they be in a separate patch?
- macro_rules! format { ($($f:tt)*) => ({
&*String::from_fmt(kernel::fmt!($($f)*))
CString::try_from_fmt(kernel::fmt!($($f)*))?.to_str()? })@@ -613,66 +597,72 @@ macro_rules! format { \xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff";
#[test]
- fn test_cstr_to_str() {
- fn test_cstr_to_str() -> Result { let good_bytes = b"\xf0\x9f\xa6\x80\0";
let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap();let checked_str = checked_cstr.to_str().unwrap();
let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?;let checked_str = checked_cstr.to_str()?; assert_eq!(checked_str, "🦀");Ok(())}
#[test]
- fn test_cstr_to_str_invalid_utf8() {
- fn test_cstr_to_str_invalid_utf8() -> Result { let bad_bytes = b"\xc3\x28\0";
let checked_cstr = CStr::from_bytes_with_nul(bad_bytes).unwrap();
let checked_cstr = CStr::from_bytes_with_nul(bad_bytes)?; assert!(checked_cstr.to_str().is_err());Ok(())}
#[test]
- fn test_cstr_as_str_unchecked() {
- fn test_cstr_as_str_unchecked() -> Result { let good_bytes = b"\xf0\x9f\x90\xA7\0";
let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap();
let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?; // SAFETY: The contents come from a string literal which contains valid UTF-8. let unchecked_str = unsafe { checked_cstr.as_str_unchecked() }; assert_eq!(unchecked_str, "🐧");Ok(())}
#[test]
- fn test_cstr_display() {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap();
- fn test_cstr_display() -> Result {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; assert_eq!(format!("{}", hello_world), "hello, world!");
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0").unwrap();
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; assert_eq!(format!("{}", non_printables), "\\x01\\x09\\x0a");
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0").unwrap();
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; assert_eq!(format!("{}", non_ascii), "d\\xe9j\\xe0 vu");
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0").unwrap();
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; assert_eq!(format!("{}", good_bytes), "\\xf0\\x9f\\xa6\\x80");Ok(())}
#[test]
- fn test_cstr_display_all_bytes() {
- fn test_cstr_display_all_bytes() -> Result { let mut bytes: [u8; 256] = [0; 256]; // fill `bytes` with [1..=255] + [0] for i in u8::MIN..=u8::MAX { bytes[i as usize] = i.wrapping_add(1); }
let cstr = CStr::from_bytes_with_nul(&bytes).unwrap();
let cstr = CStr::from_bytes_with_nul(&bytes)?; assert_eq!(format!("{}", cstr), ALL_ASCII_CHARS);Ok(())}
#[test]
- fn test_cstr_debug() {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap();
- fn test_cstr_debug() -> Result {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; assert_eq!(format!("{:?}", hello_world), "\"hello, world!\"");
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0").unwrap();
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; assert_eq!(format!("{:?}", non_printables), "\"\\x01\\x09\\x0a\"");
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0").unwrap();
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; assert_eq!(format!("{:?}", non_ascii), "\"d\\xe9j\\xe0 vu\"");
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0").unwrap();
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; assert_eq!(format!("{:?}", good_bytes), "\"\\xf0\\x9f\\xa6\\x80\"");Ok(())}
#[test]
- fn test_bstr_display() {
- fn test_bstr_display() -> Result { let hello_world = BStr::from_bytes(b"hello, world!"); assert_eq!(format!("{}", hello_world), "hello, world!"); let escapes = BStr::from_bytes(b"_\t_\n_\r_\_'_"_");
@@ -683,10 +673,11 @@ fn test_bstr_display() { assert_eq!(format!("{}", non_ascii), "d\xe9j\xe0 vu"); let good_bytes = BStr::from_bytes(b"\xf0\x9f\xa6\x80"); assert_eq!(format!("{}", good_bytes), "\xf0\x9f\xa6\x80");
Ok(())}
#[test]
- fn test_bstr_debug() {
- fn test_bstr_debug() -> Result { let hello_world = BStr::from_bytes(b"hello, world!"); assert_eq!(format!("{:?}", hello_world), ""hello, world!""); let escapes = BStr::from_bytes(b"_\t_\n_\r_\_'_"_");
@@ -697,6 +688,7 @@ fn test_bstr_debug() { assert_eq!(format!("{:?}", non_ascii), ""d\xe9j\xe0 vu""); let good_bytes = BStr::from_bytes(b"\xf0\x9f\xa6\x80"); assert_eq!(format!("{:?}", good_bytes), ""\xf0\x9f\xa6\x80"");
Ok(())}
-- 2.49.0
On Sun, May 4, 2025 at 7:30 PM Tamir Duberstein tamird@gmail.com wrote:
Alice pointed this out in another thread but: one of the downsides of returning Result is that in the event of failure the line number where the error occurred is no longer contained in the test output. I'm 👎 on this change for that reason.
We could perhaps customize `?` to help here, e.g. printing a trace or panic, with the `Try` trait or similar.
Related to this: I thought about saying in the guidelines that `?` in tests is intended for things that you would normally use `?` in similar kernel code, i.e. things that the test is not "testing", rather than things that you would want to assert explicitly. Thus the actual code under test should still have `assert!`s in the right places. I did that in the sample. That way, having `?` would still simplify a lot of test code and yet allow us to differentiate between code under test vs. other code failing.
These changes don't depend on returning `Result` from the tests AFAICT. Can they be in a separate patch?
Not sure what you mean. The change below uses `?`, which is what allows this to be removed.
Thanks!
Cheers, Miguel
On Sun, May 4, 2025 at 2:15 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, May 4, 2025 at 7:30 PM Tamir Duberstein tamird@gmail.com wrote:
Alice pointed this out in another thread but: one of the downsides of returning Result is that in the event of failure the line number where the error occurred is no longer contained in the test output. I'm 👎 on this change for that reason.
We could perhaps customize `?` to help here, e.g. printing a trace or panic, with the `Try` trait or similar.
Related to this: I thought about saying in the guidelines that `?` in tests is intended for things that you would normally use `?` in similar kernel code, i.e. things that the test is not "testing", rather than things that you would want to assert explicitly. Thus the actual code under test should still have `assert!`s in the right places. I did that in the sample. That way, having `?` would still simplify a lot of test code and yet allow us to differentiate between code under test vs. other code failing.
I see. Up to you, obviously, but ISTM that this degree of freedom is unnecessary, but perhaps there's a benefit I'm underappreciating?
These changes don't depend on returning `Result` from the tests AFAICT. Can they be in a separate patch?
Not sure what you mean. The change below uses `?`, which is what allows this to be removed.
Even without this change, couldn't you apply
macro_rules! format { ($($f:tt)*) => ({ - &*String::from_fmt(kernel::fmt!($($f)*)) + CString::try_from_fmt(kernel::fmt!($($f)*)).unwrap().to_str().unwrap() }) }
and achieve roughly the same reduction in line count in the test module?
Cheers. Tamir
On Sun, May 4, 2025 at 8:23 PM Tamir Duberstein tamird@gmail.com wrote:
I see. Up to you, obviously, but ISTM that this degree of freedom is unnecessary, but perhaps there's a benefit I'm underappreciating?
Well, having this allows one to write code like the rest of the kernel code, instead of, say, `.unwrap()`ing or `assert!`ing everywhere.
So easier to read, easier to copy-paste from normal code, and people (especially those learning) wouldn't get accustomed to seeing `.unwrap()`s etc. everywhere.
Having said that, C KUnit uses the macros for things that require stopping the test, even if "unrelated" to the actual test, and it does not look like normal code, of course. They do have `->init()` which can return a failure, but not the test cases themselves.
David perhaps has some advice here. Perhaps test functions being fallible (like returning `int`) were considered (or asserts for "unrelated" things) for C at some point and discarded.
The custom `?` is quite tempting, to get the best of both worlds, assuming we could make it work well.
Even without this change, couldn't you apply
macro_rules! format { ($($f:tt)*) => ({
&*String::from_fmt(kernel::fmt!($($f)*))
CString::try_from_fmt(kernel::fmt!($($f)*)).unwrap().to_str().unwrap() })and achieve roughly the same reduction in line count in the test module?
Sure, the line would need to change again later, but that is fine too, we can do a separate commit.
Cheers, Miguel
On Mon, 5 May 2025 at 05:54, Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, May 4, 2025 at 8:23 PM Tamir Duberstein tamird@gmail.com wrote:
I see. Up to you, obviously, but ISTM that this degree of freedom is unnecessary, but perhaps there's a benefit I'm underappreciating?
Well, having this allows one to write code like the rest of the kernel code, instead of, say, `.unwrap()`ing or `assert!`ing everywhere.
So easier to read, easier to copy-paste from normal code, and people (especially those learning) wouldn't get accustomed to seeing `.unwrap()`s etc. everywhere.
Having said that, C KUnit uses the macros for things that require stopping the test, even if "unrelated" to the actual test, and it does not look like normal code, of course. They do have `->init()` which can return a failure, but not the test cases themselves.
David perhaps has some advice here. Perhaps test functions being fallible (like returning `int`) were considered (or asserts for "unrelated" things) for C at some point and discarded.
The decision to not have a return value for tests predates me (if Brendan's around, maybe he recalls the original reason here), but I suspect it was mostly in order to encourage explicit assertions, which could then contain the line number of the failure.
We use the KUnit assertions for "unrelated" failures as well mainly as that's the only way to end a test early if it's not entirely contained in one function. But it's not _wrong_ for a test to mark itself as failed (or skipped) and then return early if that makes more sense.
The custom `?` is quite tempting, to get the best of both worlds, assuming we could make it work well.
I'm definitely a fan of using '?' over unwrap() here, if only because it won't trigger an actual panic. That being said, if it turns out to be easier to have a variant of unwrap(), that'd work, too.
The fact that not all Result Errs implement Debug makes having a nice way of printing it a bit more fun, too.
-- David
On Sat, 3 May 2025 at 05:52, Miguel Ojeda ojeda@kernel.org wrote:
Since now we have support for returning `-> Result`s, we can convert some of these tests to use the feature, and serve as a first user for it too.
Thus convert them.
This, in turn, simplifies them a fair bit.
We keep the actual assertions we want to make as explicit ones with `assert*!`s.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
I prefer this, even though I actually don't mind the .unwrap(), just because we'll funnel failures to KUnit properly, rather than having a full panic from the unwrap.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
rust/kernel/str.rs | 68 ++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 38 deletions(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index cf2caa2db168..8dcfb11013f2 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -576,25 +576,9 @@ macro_rules! c_str { mod tests { use super::*;
- struct String(CString);
- impl String {
fn from_fmt(args: fmt::Arguments<'_>) -> Self {String(CString::try_from_fmt(args).unwrap())}- }
- impl Deref for String {
type Target = str;fn deref(&self) -> &str {self.0.to_str().unwrap()}- }
- macro_rules! format { ($($f:tt)*) => ({
&*String::from_fmt(kernel::fmt!($($f)*))
CString::try_from_fmt(kernel::fmt!($($f)*))?.to_str()? })@@ -613,66 +597,72 @@ macro_rules! format { \xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff";
#[test]
- fn test_cstr_to_str() {
- fn test_cstr_to_str() -> Result { let good_bytes = b"\xf0\x9f\xa6\x80\0";
let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap();let checked_str = checked_cstr.to_str().unwrap();
let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?;let checked_str = checked_cstr.to_str()?; assert_eq!(checked_str, "🦀");Ok(())}
#[test]
- fn test_cstr_to_str_invalid_utf8() {
- fn test_cstr_to_str_invalid_utf8() -> Result { let bad_bytes = b"\xc3\x28\0";
let checked_cstr = CStr::from_bytes_with_nul(bad_bytes).unwrap();
let checked_cstr = CStr::from_bytes_with_nul(bad_bytes)?; assert!(checked_cstr.to_str().is_err());Ok(())}
#[test]
- fn test_cstr_as_str_unchecked() {
- fn test_cstr_as_str_unchecked() -> Result { let good_bytes = b"\xf0\x9f\x90\xA7\0";
let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap();
let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?; // SAFETY: The contents come from a string literal which contains valid UTF-8. let unchecked_str = unsafe { checked_cstr.as_str_unchecked() }; assert_eq!(unchecked_str, "🐧");Ok(())}
#[test]
- fn test_cstr_display() {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap();
- fn test_cstr_display() -> Result {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; assert_eq!(format!("{}", hello_world), "hello, world!");
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0").unwrap();
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; assert_eq!(format!("{}", non_printables), "\\x01\\x09\\x0a");
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0").unwrap();
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; assert_eq!(format!("{}", non_ascii), "d\\xe9j\\xe0 vu");
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0").unwrap();
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; assert_eq!(format!("{}", good_bytes), "\\xf0\\x9f\\xa6\\x80");Ok(())}
#[test]
- fn test_cstr_display_all_bytes() {
- fn test_cstr_display_all_bytes() -> Result { let mut bytes: [u8; 256] = [0; 256]; // fill `bytes` with [1..=255] + [0] for i in u8::MIN..=u8::MAX { bytes[i as usize] = i.wrapping_add(1); }
let cstr = CStr::from_bytes_with_nul(&bytes).unwrap();
let cstr = CStr::from_bytes_with_nul(&bytes)?; assert_eq!(format!("{}", cstr), ALL_ASCII_CHARS);Ok(())}
#[test]
- fn test_cstr_debug() {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap();
- fn test_cstr_debug() -> Result {
let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; assert_eq!(format!("{:?}", hello_world), "\"hello, world!\"");
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0").unwrap();
let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; assert_eq!(format!("{:?}", non_printables), "\"\\x01\\x09\\x0a\"");
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0").unwrap();
let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; assert_eq!(format!("{:?}", non_ascii), "\"d\\xe9j\\xe0 vu\"");
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0").unwrap();
let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; assert_eq!(format!("{:?}", good_bytes), "\"\\xf0\\x9f\\xa6\\x80\"");Ok(())}
#[test]
- fn test_bstr_display() {
- fn test_bstr_display() -> Result { let hello_world = BStr::from_bytes(b"hello, world!"); assert_eq!(format!("{}", hello_world), "hello, world!"); let escapes = BStr::from_bytes(b"_\t_\n_\r_\_'_"_");
@@ -683,10 +673,11 @@ fn test_bstr_display() { assert_eq!(format!("{}", non_ascii), "d\xe9j\xe0 vu"); let good_bytes = BStr::from_bytes(b"\xf0\x9f\xa6\x80"); assert_eq!(format!("{}", good_bytes), "\xf0\x9f\xa6\x80");
Ok(())}
#[test]
- fn test_bstr_debug() {
- fn test_bstr_debug() -> Result { let hello_world = BStr::from_bytes(b"hello, world!"); assert_eq!(format!("{:?}", hello_world), ""hello, world!""); let escapes = BStr::from_bytes(b"_\t_\n_\r_\_'_"_");
@@ -697,6 +688,7 @@ fn test_bstr_debug() { assert_eq!(format!("{:?}", non_ascii), ""d\xe9j\xe0 vu""); let good_bytes = BStr::from_bytes(b"\xf0\x9f\xa6\x80"); assert_eq!(format!("{:?}", good_bytes), ""\xf0\x9f\xa6\x80"");
Ok(())}
-- 2.49.0
Now that `rusttest`s are not really used much, clarify the section of the documentation that describes them.
In addition, free the section name for the KUnit-based `#[test]`s that will be added afterwards. To do so, rename the section into `rusttest` host tests.
Signed-off-by: Miguel Ojeda ojeda@kernel.org --- Documentation/rust/testing.rst | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/Documentation/rust/testing.rst b/Documentation/rust/testing.rst index f692494f7b74..6337b83815ab 100644 --- a/Documentation/rust/testing.rst +++ b/Documentation/rust/testing.rst @@ -130,16 +130,17 @@ please see:
https://rust.docs.kernel.org/kernel/error/type.Result.html#error-codes-in-c-...
-The ``#[test]`` tests ---------------------- +The ``rusttest`` host tests +---------------------------
-Additionally, there are the ``#[test]`` tests. These can be run using the -``rusttest`` Make target:: +These are userspace tests that can be built and run in the host (i.e. the one +that performs the kernel build) using the ``rusttest`` Make target::
make LLVM=1 rusttest
-This requires the kernel ``.config``. It runs the ``#[test]`` tests on the host -(currently) and thus is fairly limited in what these tests can test. +This requires the kernel ``.config``. + +Currently, they are mostly used for testing the ``macros`` crate's examples.
The Kselftests --------------
On Sat, 3 May 2025 at 05:52, Miguel Ojeda ojeda@kernel.org wrote:
Now that `rusttest`s are not really used much, clarify the section of the documentation that describes them.
In addition, free the section name for the KUnit-based `#[test]`s that will be added afterwards. To do so, rename the section into `rusttest` host tests.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
Documentation/rust/testing.rst | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/Documentation/rust/testing.rst b/Documentation/rust/testing.rst index f692494f7b74..6337b83815ab 100644 --- a/Documentation/rust/testing.rst +++ b/Documentation/rust/testing.rst @@ -130,16 +130,17 @@ please see:
https://rust.docs.kernel.org/kernel/error/type.Result.html#error-codes-in-c-and-rust-The ``#[test]`` tests
+The ``rusttest`` host tests +---------------------------
-Additionally, there are the ``#[test]`` tests. These can be run using the -``rusttest`` Make target:: +These are userspace tests that can be built and run in the host (i.e. the one +that performs the kernel build) using the ``rusttest`` Make target::
make LLVM=1 rusttest-This requires the kernel ``.config``. It runs the ``#[test]`` tests on the host -(currently) and thus is fairly limited in what these tests can test. +This requires the kernel ``.config``.
+Currently, they are mostly used for testing the ``macros`` crate's examples.
The Kselftests
-- 2.49.0
There was no documentation yet on the KUnit-based `#[test]`s.
Thus add it now.
It includes an explanation about the `assert*!` macros being mapped to KUnit and the support for `-> Result` introduced in these series.
Signed-off-by: Miguel Ojeda ojeda@kernel.org --- Documentation/rust/testing.rst | 71 ++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+)
diff --git a/Documentation/rust/testing.rst b/Documentation/rust/testing.rst index 6337b83815ab..f43cb77bcc69 100644 --- a/Documentation/rust/testing.rst +++ b/Documentation/rust/testing.rst @@ -130,6 +130,77 @@ please see:
https://rust.docs.kernel.org/kernel/error/type.Result.html#error-codes-in-c-...
+The ``#[test]`` tests +--------------------- + +Additionally, there are the ``#[test]`` tests. Like for documentation tests, +these are also fairly similar to what you would expect from userspace, and they +are also mapped to KUnit. + +These tests are introduced by the ``kunit_tests`` procedural macro, which takes +the name of the test suite as an argument. + +For instance, assume we want to test the function ``f`` from the documentation +tests section. We could write, in the same file where we have our function: + +.. code-block:: rust + + #[kunit_tests(rust_kernel_mymod)] + mod tests { + use super::*; + + #[test] + fn test_f() { + assert_eq!(f(10, 20), 30); + } + } + +And if we run it, the kernel log would look like:: + + KTAP version 1 + # Subtest: rust_kernel_mymod + # speed: normal + 1..1 + # test_f.speed: normal + ok 1 test_f + ok 1 rust_kernel_mymod + +Like documentation tests, the ``assert!`` and ``assert_eq!`` macros are mapped +back to KUnit and do not panic. Similarly, the +`? https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator`_ +operator is supported, i.e. the test functions may return either nothing (i.e. +the unit type ``()``) or ``Result`` (i.e. any ``Result<T, E>``). For instance: + +.. code-block:: rust + + #[kunit_tests(rust_kernel_mymod)] + mod tests { + use super::*; + + #[test] + fn test_g() -> Result { + let x = g()?; + assert_eq!(x, 30); + Ok(()) + } + } + +If we run the test and the call to ``g`` fails, then the kernel log would show:: + + KTAP version 1 + # Subtest: rust_kernel_mymod + # speed: normal + 1..1 + # test_g: ASSERTION FAILED at rust/kernel/lib.rs:335 + Expected is_test_result_ok(test_g()) to be true, but is false + # test_g.speed: normal + not ok 1 test_g + not ok 1 rust_kernel_mymod + +If a ``#[test]`` test could be useful as an example for the user, then please +use a documentation test instead. Even edge cases of an API, e.g. error or +boundary cases, can be interesting to show in examples. + The ``rusttest`` host tests ---------------------------
On Sat, 3 May 2025 at 05:52, Miguel Ojeda ojeda@kernel.org wrote:
There was no documentation yet on the KUnit-based `#[test]`s.
Thus add it now.
It includes an explanation about the `assert*!` macros being mapped to KUnit and the support for `-> Result` introduced in these series.
Signed-off-by: Miguel Ojeda ojeda@kernel.org
Assuming all of the other changes go through, this looks good to me.
It _may_ be useful to add some notes about when to choose KUnit tests versus rusttest host tests: particularly around cross-compiling and/or the need to call kernel APIs / access global kernel state. But some of that is covered in the general kernel testing / KUnit documentation in Documentation/dev-tools, anyway.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
Documentation/rust/testing.rst | 71 ++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+)
diff --git a/Documentation/rust/testing.rst b/Documentation/rust/testing.rst index 6337b83815ab..f43cb77bcc69 100644 --- a/Documentation/rust/testing.rst +++ b/Documentation/rust/testing.rst @@ -130,6 +130,77 @@ please see:
https://rust.docs.kernel.org/kernel/error/type.Result.html#error-codes-in-c-and-rust+The ``#[test]`` tests +---------------------
+Additionally, there are the ``#[test]`` tests. Like for documentation tests, +these are also fairly similar to what you would expect from userspace, and they +are also mapped to KUnit.
+These tests are introduced by the ``kunit_tests`` procedural macro, which takes +the name of the test suite as an argument.
+For instance, assume we want to test the function ``f`` from the documentation +tests section. We could write, in the same file where we have our function:
+.. code-block:: rust
#[kunit_tests(rust_kernel_mymod)]mod tests {use super::*;#[test]fn test_f() {assert_eq!(f(10, 20), 30);}}+And if we run it, the kernel log would look like::
KTAP version 1# Subtest: rust_kernel_mymod# speed: normal1..1# test_f.speed: normalok 1 test_fok 1 rust_kernel_mymod+Like documentation tests, the ``assert!`` and ``assert_eq!`` macros are mapped +back to KUnit and do not panic. Similarly, the +`? https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator`_ +operator is supported, i.e. the test functions may return either nothing (i.e. +the unit type ``()``) or ``Result`` (i.e. any ``Result<T, E>``). For instance:
+.. code-block:: rust
#[kunit_tests(rust_kernel_mymod)]mod tests {use super::*;#[test]fn test_g() -> Result {let x = g()?;assert_eq!(x, 30);Ok(())}}+If we run the test and the call to ``g`` fails, then the kernel log would show::
KTAP version 1# Subtest: rust_kernel_mymod# speed: normal1..1# test_g: ASSERTION FAILED at rust/kernel/lib.rs:335Expected is_test_result_ok(test_g()) to be true, but is false# test_g.speed: normalnot ok 1 test_gnot ok 1 rust_kernel_mymod+If a ``#[test]`` test could be useful as an example for the user, then please +use a documentation test instead. Even edge cases of an API, e.g. error or +boundary cases, can be interesting to show in examples.
The ``rusttest`` host tests
-- 2.49.0
On Fri, May 02, 2025 at 11:51:25PM +0200, Miguel Ojeda wrote:
Improvements that build on top of the very basic `#[test]` support merged in v6.15.
They are fairly minimal changes, but they allow us to map `assert*!`s back to KUnit, plus to add support for test functions that return `Result`s.
In essence, they get our `#[test]`s essentially on par with the documentation tests.
I also took the chance to convert some host `#[test]`s we had to KUnit in order to showcase the feature.
Finally, I added documentation that was lacking from the original submission.
I hope this helps.
It does -- thanks for this series!
Acked-by: Danilo Krummrich dakr@kernel.org
rust: str: convert `rusttest` tests into KUnit
With that, do we still expose `alloc` primitives to userspace tests?
On Mon, May 5, 2025 at 6:57 PM Danilo Krummrich dakr@kernel.org wrote:
With that, do we still expose `alloc` primitives to userspace tests?
I considered removing a bunch of stuff (even the build support for non-`macros` `rusttest`, to be honest) -- you are referring to the `any(test, testlib)` bits, right?
I think we can wait to see if we need it, or we can also just remove it and re-introduce later if needed.
Thanks for taking a look!
Cheers, Miguel
linux-kselftest-mirror@lists.linaro.org
-
Boqun Feng -
Danilo Krummrich -
David Gow -
John Hubbard -
Miguel Ojeda -
Miguel Ojeda -
Tamir Duberstein