Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 14 Jun 2023 20:38:03 +0000
In-Reply-To: <20230614180837.630180-6-ojeda@kernel.org>
Mime-Version: 1.0
References: <20230614180837.630180-6-ojeda@kernel.org>
Message-ID: <20230614203803.2910957-1-aliceryhl@google.com>
Subject: Re: [PATCH 5/6] rust: support running Rust documentation tests as
 KUnit ones
From:   Alice Ryhl <aliceryhl@google.com>
To:     ojeda@kernel.org
Cc:     alex.gaynor@gmail.com, aliceryhl@google.com,
        benno.lossin@proton.me, bjorn3_gh@protonmail.com,
        boqun.feng@gmail.com, brendan.higgins@linux.dev,
        davidgow@google.com, gary@garyguo.net, kunit-dev@googlegroups.com,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        nmi@metaspace.dk, patches@lists.linux.dev, philip.li@intel.com,
        rust-for-linux@vger.kernel.org, wedsonaf@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Miguel Ojeda <ojeda@kernel.org> writes:
> Rust has documentation tests: these are typically examples of
> usage of any item (e.g. function, struct, module...).
> 
> They are very convenient because they are just written
> alongside the documentation. For instance:
> 
>     /// Sums two numbers.
>     ///
>     /// ```
>     /// assert_eq!(mymod::f(10, 20), 30);
>     /// ```
>     pub fn f(a: i32, b: i32) -> i32 {
>         a + b
>     }
> 
> In userspace, the tests are collected and run via `rustdoc`.
> Using the tool as-is would be useful already, since it allows
> to compile-test most tests (thus enforcing they are kept
> in sync with the code they document) and run those that do not
> depend on in-kernel APIs.
> 
> However, by transforming the tests into a KUnit test suite,
> they can also be run inside the kernel. Moreover, the tests
> get to be compiled as other Rust kernel objects instead of
> targeting userspace.
> 
> On top of that, the integration with KUnit means the Rust
> support gets to reuse the existing testing facilities. For
> instance, the kernel log would look like:
> 
>     KTAP version 1
>     1..1
>         KTAP version 1
>         # Subtest: rust_doctests_kernel
>         1..59
>         # Doctest from line 13
>         ok 1 rust_doctest_kernel_build_assert_rs_0
>         # Doctest from line 56
>         ok 2 rust_doctest_kernel_build_assert_rs_1
>         # Doctest from line 122
>         ok 3 rust_doctest_kernel_init_rs_0
>         ...
>         # Doctest from line 150
>         ok 59 rust_doctest_kernel_types_rs_2
>     # rust_doctests_kernel: pass:59 fail:0 skip:0 total:59
>     # Totals: pass:59 fail:0 skip:0 total:59
>     ok 1 rust_doctests_kernel
> 
> Therefore, add support for running Rust documentation tests
> in KUnit. Some other notes about the current implementation
> and support follow.
> 
> The transformation is performed by a couple scripts written
> as Rust hostprogs.
> 
> Tests using the `?` operator are also supported as usual, e.g.:
> 
>     /// ```
>     /// # use kernel::{spawn_work_item, workqueue};
>     /// spawn_work_item!(workqueue::system(), || pr_info!("x"))?;
>     /// # Ok::<(), Error>(())
>     /// ```
> 
> The tests are also compiled with Clippy under `CLIPPY=1`, just like
> normal code, thus also benefitting from extra linting.
> 
> The names of the tests are currently automatically generated.
> This allows to reduce the burden for documentation writers,
> while keeping them fairly stable for bisection. This is an
> improvement over the `rustdoc`-generated names, which include
> the line number; but ideally we would like to get `rustdoc` to
> provide the Rust item path and a number (for multiple examples
> in a single documented Rust item).
> 
> In order for developers to easily see from which original line
> a failed doctests came from, a KTAP diagnostic line is printed
> to the log. In the future, we may be able to use a proper KUnit
> facility to append this sort of information instead.
> 
> A notable difference from KUnit C tests is that the Rust tests
> appear to assert using the usual `assert!` and `assert_eq!`
> macros from the Rust standard library (`core`). We provide
> a custom version that forwards the call to KUnit instead.
> Importantly, these macros do not require passing context,
> unlike the KUnit C ones (i.e. `struct kunit *`). This makes
> them easier to use, and readers of the documentation do not need
> to care about which testing framework is used. In addition, it
> may allow us to test third-party code more easily in the future.
> 
> However, a current limitation is that KUnit does not support
> assertions in other tasks. Thus we presently simply print an
> error to the kernel log if an assertion actually failed. This
> should be revisited to properly fail the test, perhaps saving
> the context somewhere else, or letting KUnit handle it.
> 
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

(One nit later below.)

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +fn main() {
> +    let mut stdin = std::io::stdin().lock();
> +    let mut body = String::new();
> +    stdin.read_to_string(&mut body).unwrap();
> +
> +    // Find the generated function name looking for the inner function inside `main()`.
> +    //
> +    // The line we are looking for looks like one of the following:
> +    //
> +    // ```
> +    // fn main() { #[allow(non_snake_case)] fn _doctest_main_rust_kernel_file_rs_28_0() {
> +    // fn main() { #[allow(non_snake_case)] fn _doctest_main_rust_kernel_file_rs_37_0() -> Result<(), impl core::fmt::Debug> {
> +    // ```
> +    //
> +    // It should be unlikely that doctest code matches such lines (when code is formatted properly).
> +    let rustdoc_function_name = body
> +        .lines()
> +        .find_map(|line| {
> +            Some(
> +                line.split_once("fn main() {")?
> +                    .1
> +                    .split_once("fn ")?
> +                    .1
> +                    .split_once("()")?
> +                    .0,
> +            )
> +            .filter(|x| x.chars().all(|c| c.is_alphanumeric() || c == '_'))
> +        })
> +        .expect("No test function found in `rustdoc`'s output.");
> +
> +    // Qualify `Result` to avoid the collision with our own `Result` coming from the prelude.
> +    let body = body.replace(
> +        &format!("{rustdoc_function_name}() -> Result<(), impl core::fmt::Debug> {{"),
> +        &format!("{rustdoc_function_name}() -> core::result::Result<(), impl core::fmt::Debug> {{"),
> +    );
> +
> +    // For tests that get generated with `Result`, like above, `rustdoc` generates an `unwrap()` on
> +    // the return value to check there were no returned errors. Instead, we use our assert macro
> +    // since we want to just fail the test, not panic the kernel.
> +    //
> +    // We save the result in a variable so that the failed assertion message looks nicer.
> +    let body = body.replace(
> +        &format!("}} {rustdoc_function_name}().unwrap() }}"),
> +        &format!("}} let test_return_value = {rustdoc_function_name}(); assert!(test_return_value.is_ok()); }}"),
> +    );
> +
> +    // Figure out a smaller test name based on the generated function name.
> +    let name = rustdoc_function_name.split_once("_rust_kernel_").unwrap().1;
> +
> +    let path = format!("rust/test/doctests/kernel/{name}");
> +
> +    write!(BufWriter::new(File::create(path).unwrap()), "{body}").unwrap();

This could just be

std::fs::write(path, body.as_bytes());

Alice