| Message ID | 20230614180837.630180-1-ojeda@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | KUnit integration for Rust doctests | expand |
On Wed, Jun 14, 2023 at 08:08:24PM +0200, Miguel Ojeda wrote: > This is the initial KUnit integration for running Rust documentation > tests within the kernel. > > Thank you to the KUnit team for all the input and feedback on this > over the months, as well as the Intel LKP 0-Day team! > > This may be merged through either the KUnit or the Rust trees. If > the KUnit team wants to merge it, then that would be great. > > Please see the message in the main commit for the details. > Great work! I've played this for a while, and it's really useful ;-) One thing though, maybe we can provide more clues for users to locate the corresponding Doctests? For example, I did the following to trigger an assertion: diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs index 91eb2c9e9123..9ead152e2c7e 100644 --- a/rust/kernel/sync/lock/spinlock.rs +++ b/rust/kernel/sync/lock/spinlock.rs @@ -58,7 +58,7 @@ macro_rules! new_spinlock { /// /// // Allocate a boxed `Example`. /// let e = Box::pin_init(Example::new())?; -/// assert_eq!(e.c, 10); +/// assert_eq!(e.c, 11); /// assert_eq!(e.d.lock().a, 20); /// assert_eq!(e.d.lock().b, 30); /// # Ok::<(), Error>(()) Originally I got: [..] # Doctest from line 35 [..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/doctests_kernel_generated.rs:2437 [..] Expected e.c == 11 to be true, but is false [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0 The assertion warning only says line 35 but which file? Yes, the ".._sync_lock_spinlock_rs" name does provide the lead, however since we generate the test code, so we actually know the line # for each real test body, so I come up a way to give us the following: [..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61 [..] Expected e.c == 11 to be true, but is false [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0 Thoughts? Regards, Boqun ----------------->8 diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 3c94efcd7f76..807fe3633567 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -49,15 +49,15 @@ pub fn info(args: fmt::Arguments<'_>) { #[doc(hidden)] #[macro_export] macro_rules! kunit_assert { - ($name:literal, $condition:expr $(,)?) => { + ($name:literal, $diff:expr, $file:expr, $condition:expr $(,)?) => { 'out: { // Do nothing if the condition is `true`. if $condition { break 'out; } - static LINE: i32 = core::line!() as i32; - static FILE: &'static $crate::str::CStr = $crate::c_str!(core::file!()); + static LINE: i32 = core::line!() as i32 - $diff; + static FILE: &'static $crate::str::CStr = $crate::c_str!($file); static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition)); // SAFETY: FFI call without safety requirements. @@ -148,9 +148,9 @@ unsafe impl Sync for UnaryAssert {} #[doc(hidden)] #[macro_export] macro_rules! kunit_assert_eq { - ($name:literal, $left:expr, $right:expr $(,)?) => {{ + ($name:literal, $diff:expr, $file:expr, $left:expr, $right:expr $(,)?) => {{ // For the moment, we just forward to the expression assert because, for binary asserts, // KUnit supports only a few types (e.g. integers). - $crate::kunit_assert!($name, $left == $right); + $crate::kunit_assert!($name, $diff, $file, $left == $right); }}; } diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index 793885c32c0d..4786a2ef0dc6 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -75,6 +75,11 @@ fn main() { let line = line.parse::<core::ffi::c_int>().unwrap(); + let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/")); + + // Calculate how many lines before `main` function (including the `main` function line). + let body_offset = body.lines().take_while(|l| !l.contains("fn main() {")).count() + 1; + use std::fmt::Write; write!( rust_tests, @@ -85,7 +90,7 @@ pub extern "C" fn {kunit_name}(__kunit_test: *mut kernel::bindings::kunit) {{ #[allow(unused)] macro_rules! assert {{ ($cond:expr $(,)?) => {{{{ - kernel::kunit_assert!("{kunit_name}", $cond); + kernel::kunit_assert!("{kunit_name}", anchor - {line}, "{src_file}", $cond); }}}} }} @@ -93,7 +98,7 @@ macro_rules! assert {{ #[allow(unused)] macro_rules! assert_eq {{ ($left:expr, $right:expr $(,)?) => {{{{ - kernel::kunit_assert_eq!("{kunit_name}", $left, $right); + kernel::kunit_assert_eq!("{kunit_name}", anchor - {line}, "{src_file}", $left, $right); }}}} }} @@ -101,9 +106,8 @@ macro_rules! assert_eq {{ #[allow(unused)] use kernel::prelude::*; - // Display line number so that developers can map the test easily to the source code. - kernel::kunit::info(format_args!(" # Doctest from line {line}\n")); - + // The anchor where the test code body starts. + static anchor: i32 = core::line!() as i32 + {body_offset} + 1; {{ {body} main();
On Thu, Jun 15, 2023 at 3:44 AM Boqun Feng <boqun.feng@gmail.com> wrote: > > Great work! I've played this for a while, and it's really useful ;-) Thanks! > The assertion warning only says line 35 but which file? Yes, the > ".._sync_lock_spinlock_rs" name does provide the lead, however since we > generate the test code, so we actually know the line # for each real > test body, so I come up a way to give us the following: > > [..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61 > [..] Expected e.c == 11 to be true, but is false > [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0 > > Thoughts? Sounds good to me. However, David/Philip, is this OK or do you really need/use the actual/compiled source file there? If you don't need it, does it need to exist / be a real file at least? If the latter answer is a "yes", which I guess it may be likely, then: > + let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/")); This would not work for files with a `_` in their name, like `locked_by.rs`. I guess we could still find the real filename based on that information walking the dir, which is another hack I recall considering at some point. Otherwise, if "fake" filenames in the line above are OK for David/Philip (I suspect they may want to open them for reporting?), then I guess the `file` one may be good enough and eventually we should get `rustdoc` to give us the proper metadata anyway. Cheers, Miguel
On Thu, 15 Jun 2023 at 16:20, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Thu, Jun 15, 2023 at 3:44 AM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Great work! I've played this for a while, and it's really useful ;-) > > Thanks! > > > The assertion warning only says line 35 but which file? Yes, the > > ".._sync_lock_spinlock_rs" name does provide the lead, however since we > > generate the test code, so we actually know the line # for each real > > test body, so I come up a way to give us the following: > > > > [..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61 > > [..] Expected e.c == 11 to be true, but is false > > [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0 > > > > Thoughts? I like it. A part of me would like to keep the kernel::kunit::info(format_args!(" # Doctest from line {line}\n")); If only so we can preserve the line information when the tests actually pass. This is something that'd probably ultimately fit as a "test attribute": https://lore.kernel.org/linux-kselftest/20230610005149.1145665-1-rmoar@google.com/ For C tests we're not bothering outputting line numbers now (though again, are considering if we can do it with an attribute), but those tests have much more searchable names, so I think it still makes sense for the Rust ones. How about printing something like: # source_line: {} kunit.py will hide this when the test passes unless the user explicitly passes --raw_output. > > Sounds good to me. However, David/Philip, is this OK or do you really > need/use the actual/compiled source file there? If you don't need it, > does it need to exist / be a real file at least? If the latter answer > is a "yes", which I guess it may be likely, then: I don't think there's anything automated using the file:line in assertion messages, it's just for human consumption. This may change in the future (and there are probably some text editors around which will turn a path like that into, e.g., a clickable link now), but we're not currently doing anything which would actually open the file. > > > + let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/")); > > This would not work for files with a `_` in their name, like > `locked_by.rs`. I guess we could still find the real filename based on > that information walking the dir, which is another hack I recall > considering at some point. > > Otherwise, if "fake" filenames in the line above are OK for > David/Philip (I suspect they may want to open them for reporting?), > then I guess the `file` one may be good enough and eventually we > should get `rustdoc` to give us the proper metadata anyway. Personally, I'd think a "fake filename" is okay (especially if it's temporary until we can get the right one), though I'd prefer there to be some indication that it's "fake": maybe leaving the _ separator in, or wrapping it in brackets, or something? Unless the whole walk-the-filesystem technique ends up being worth doing, so we don't have a significant chance of the filename being dud. -- David
On Thu, 15 Jun 2023 at 02:09, Miguel Ojeda <ojeda@kernel.org> wrote: > > This is the initial KUnit integration for running Rust documentation > tests within the kernel. > > Thank you to the KUnit team for all the input and feedback on this > over the months, as well as the Intel LKP 0-Day team! > > This may be merged through either the KUnit or the Rust trees. If > the KUnit team wants to merge it, then that would be great. > > Please see the message in the main commit for the details. > > Thanks very much for putting this together! I've been looking forward to it, and it works well here. I've been running it on linux-next to get both the pending KUnit and Rust changes, and it works well apart from needing to fix a couple of conflicts from https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=260755184cbdb267a046e7ffd397c1d2ba09bb5e In particular, the tests run with: ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_RUST=y --make_options LLVM=1 'rust_doctests_kernel' And also under QEMU / x86_64 with: ./tools/testing/kunit/kunit.py run --arch x86_64 --kconfig_add CONFIG_RUST=y --make_options LLVM=1 'rust_doctests_kernel' (And I'm looking forward to trying out the other architecture support patches with it, too) The doctests also run nicely as part of the default test suite when CONFIG_RUST=y. At some point, we might want to add a Rust-specific .kunitconfig to make it easier to just run Rust-related test suites, but it's not a big deal for just these. I assume we'll take this in via the kselftest/kunit tree for 6.6, but if you'd rather take them via the Rust tree, that's fine too. Cheers, -- David > Miguel Ojeda (6): > rust: init: make doctests compilable/testable > rust: str: make doctests compilable/testable > rust: sync: make doctests compilable/testable > rust: types: make doctests compilable/testable > rust: support running Rust documentation tests as KUnit ones > MAINTAINERS: add Rust KUnit files to the KUnit entry > > MAINTAINERS | 2 + > lib/Kconfig.debug | 13 +++ > rust/.gitignore | 2 + > rust/Makefile | 29 ++++++ > rust/bindings/bindings_helper.h | 1 + > rust/helpers.c | 7 ++ > rust/kernel/init.rs | 25 +++-- > rust/kernel/kunit.rs | 156 ++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 2 + > rust/kernel/str.rs | 4 +- > rust/kernel/sync/arc.rs | 9 +- > rust/kernel/sync/lock/mutex.rs | 1 + > rust/kernel/sync/lock/spinlock.rs | 1 + > rust/kernel/types.rs | 6 +- > scripts/.gitignore | 2 + > scripts/Makefile | 4 + > scripts/rustdoc_test_builder.rs | 73 ++++++++++++++ > scripts/rustdoc_test_gen.rs | 162 ++++++++++++++++++++++++++++++ > 18 files changed, 484 insertions(+), 15 deletions(-) > create mode 100644 rust/kernel/kunit.rs > create mode 100644 scripts/rustdoc_test_builder.rs > create mode 100644 scripts/rustdoc_test_gen.rs > > > base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3 > -- > 2.41.0 >