Re: [PATCH 5/6] rust: support running Rust documentation tests as KUnit ones

[Martin Rodriguez Reboredo]

On 6/14/23 15:08, Miguel Ojeda wrote:
> 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@xxxxxxxxxx>
> ---
> [...]
> diff --git a/scripts/rustdoc_test_builder.rs b/scripts/rustdoc_test_builder.rs
> new file mode 100644
> index 000000000000..e3b7138fb4f9
> --- /dev/null
> +++ b/scripts/rustdoc_test_builder.rs
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> [...]
> +
> +fn main() {
> [...]
> +
> +    write!(BufWriter::new(File::create(path).unwrap()), "{body}").unwrap();

I can't remember that if this panic it will mention the path on it.
Though if it does, then use something more explicit than
`.unwrap()`.

> +}
> diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs
> new file mode 100644
> index 000000000000..793885c32c0d
> --- /dev/null
> +++ b/scripts/rustdoc_test_gen.rs
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> [...]
> +
> +use std::io::{BufWriter, Read, Write};
> +use std::{fs, fs::File};
> +
> +fn main() {
> +    let mut paths = fs::read_dir("rust/test/doctests/kernel")
> +        .unwrap()
> +        .map(|entry| entry.unwrap().path())
> +        .collect::<Vec<_>>();
> +
> +    // Sort paths for clarity.
> +    paths.sort();
> +
> +    let mut rust_tests = String::new();
> +    let mut c_test_declarations = String::new();
> +    let mut c_test_cases = String::new();
> +    let mut body = String::new();
> +    let mut last_file = String::new();
> +    let mut number = 0;
> +    for path in paths {
> +        // The `name` follows the `{file}_{line}_{number}` pattern (see description in
> +        // `scripts/rustdoc_test_builder.rs`). Discard the `number`.
> +        let name = path.file_name().unwrap().to_str().unwrap().to_string();
> +
> +        // Extract the `file` and the `line`, discarding the `number`.
> +        let (file, line) = name.rsplit_once('_').unwrap().0.rsplit_once('_').unwrap();

Please do not use unwrap here, one can easily create a path that
it's not compliant under `rust/test/doctests/kernel` and get no
clue about where this script has failed. Use `.expect()` or
something else instead.

> +
> [...]
> +
> +    write!(
> +        BufWriter::new(File::create("rust/doctests_kernel_generated.rs").unwrap()),
> +        r#"//! `kernel` crate documentation tests.
> +
> +const __LOG_PREFIX: &[u8] = b"rust_doctests_kernel\0";
> +
> +{rust_tests}
> +"#
> +    )
> +    .unwrap();
> +
> +    write!(
> +        BufWriter::new(File::create("rust/doctests_kernel_generated_kunit.c").unwrap()),
> +        r#"/*
> + * `kernel` crate documentation tests.
> + */
> +
> +#include <kunit/test.h>
> +
> +{c_test_declarations}
> +
> +static struct kunit_case test_cases[] = {{
> +    {c_test_cases}
> +    {{ }}
> +}};
> +
> +static struct kunit_suite test_suite = {{
> +    .name = "rust_doctests_kernel",
> +    .test_cases = test_cases,
> +}};
> +
> +kunit_test_suite(test_suite);
> +
> +MODULE_LICENSE("GPL");
> +"#
> +    )
> +    .unwrap();

Same from `scripts/rustdoc_test_builder.rs` applies here.

> +}