[v2,0/7] KUnit integration for Rust doctests

  v1: https://lore.kernel.org/rust-for-linux/20230614180837.630180-1-ojeda@kernel.org/
v2:

  - Rebased on top of v6.5-rc1, which requires a change from
    `kunit_do_failed_assertion` to `__kunit_do_failed_assertion` (since
    the former became a macro) and the addition of a call to
    `__kunit_abort` (since previously the call was done by the old
    function which we cannot use anymore since it is a macro). (David)

  - Added prerequisite patch to KUnit header to include `stddef.h` to
    support the `KUNIT=y` case. (Reported by Boqun)

  - Added comment on the purpose of `trait FromErrno`. (Martin asked
    about it)

  - Simplify code to use `std::fs::write` instead of `write!`, which
    improves code size too. (Suggested by Alice)

  - Fix copy-paste type in docs from "error" to "info" and, to make it
    proper English, copy the `printk` docs style, i.e. from "info"
    to "info-level message" -- and same for the "error" one. (Miguel)

  - Swap `FILE` and `LINE` `static`s to keep the same order as done
    elsewhere. (Miguel)

  - Rename config option from `RUST_KERNEL_KUNIT_TEST` to
    `RUST_KERNEL_DOCTESTS` (and update its title), so that we can use
    the former for the "normal" (i.e. non-doctests, e.g. `#[test]` ones)
    tests in the future. (David)

  - Follow the syntax proposed for declaring test metadata in the KTAP
    v2 spec, which may also get used for the KUnit test attributes API.

    Thus, instead of "# Doctest from line {line}", use
    "# {test_name}.location: {file}.{line}", which ideally will allow to
    migrate to a KUnit attribute later.

    This is done in all cases, i.e. when the tests succeeds, because
    it may be useful for users running KUnit manually, or when passing
    `--raw_output` to `kunit.py`. (David)

    David: I used "location" instead of your suggested "line" alone, in
    order to have both in a single line, which looked nice and closer to
    the "ASSERTION FAILURE" case/line, since now we do have the original
    file (please see below).

  - Figure out the original line. This is done by deploying an anchor
    so that the difference in lines between the beginning of the test
    and the assert, in the generated file, can be computed. Then, we
    offset the line number of the original test, which is given by
    `rustdoc`. (developed by Boqun)

  - Figure out the original file. This is done by walking the
    filesystem, checking directory prefixes to reduce the amount of
    combinations to check, and it is only done once per file (rather
    than per test).

    Ambiguities are detected and reported. It does limit the filenames
    (module names) we can use, but it is unlikely we will hit it soon
    and this should be temporary anyway until `rustdoc` provides us
    with the real path. (Miguel)

    Tested with both in-tree and `O=` builds, but I would appreciate
    extra testing on this one, including via the `kunit.py` script.

  - The three last items combined means that we now see this output even
    for successful cases:

        # rust_doctest_kernel_sync_locked_by_rs_0.location: rust/kernel/sync/locked_by.rs:28
        ok 53 rust_doctest_kernel_sync_locked_by_rs_0

    Which basically gives the user all the information they need to go
    back to the source code of the doctest, while keeping them fairly
    stable for bisection, and while avoiding to require users to write
    test names manually. (David + Boqun + Miguel)

    David: from what I saw in v2 of the RFC for the test attributes API,
    the syntax still contains the test name when it is not a suite, so
    I followed that, but if you prefer to omit it, please feel free to
    do so (for me either way it is fine, and if this is the expected
    attribute syntax, I guess it is worth to follow it to make migration
    easier later on):

        # location: rust/kernel/sync/locked_by.rs:28
        ok 53 rust_doctest_kernel_sync_locked_by_rs_0

  - Collected `Reviewed-by`s and `Tested-by`s, except for the main
    commit due to the substantial changes.

Miguel Ojeda (7):
  kunit: test-bug.h: include `stddef.h` for `NULL`
  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 +
 include/kunit/test-bug.h          |   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               |  26 +--
 rust/kernel/kunit.rs              | 163 +++++++++++++++++++
 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   |  72 +++++++++
 scripts/rustdoc_test_gen.rs       | 260 ++++++++++++++++++++++++++++++
 19 files changed, 591 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: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
  

On Tue, 18 Jul 2023 at 13:28, Miguel Ojeda <ojeda@kernel.org> wrote:
>
> v1: https://lore.kernel.org/rust-for-linux/20230614180837.630180-1-ojeda@kernel.org/
> v2:
>
>   - Rebased on top of v6.5-rc1, which requires a change from
>     `kunit_do_failed_assertion` to `__kunit_do_failed_assertion` (since
>     the former became a macro) and the addition of a call to
>     `__kunit_abort` (since previously the call was done by the old
>     function which we cannot use anymore since it is a macro). (David)
>
>   - Added prerequisite patch to KUnit header to include `stddef.h` to
>     support the `KUNIT=y` case. (Reported by Boqun)
>
>   - Added comment on the purpose of `trait FromErrno`. (Martin asked
>     about it)
>
>   - Simplify code to use `std::fs::write` instead of `write!`, which
>     improves code size too. (Suggested by Alice)
>
>   - Fix copy-paste type in docs from "error" to "info" and, to make it
>     proper English, copy the `printk` docs style, i.e. from "info"
>     to "info-level message" -- and same for the "error" one. (Miguel)
>
>   - Swap `FILE` and `LINE` `static`s to keep the same order as done
>     elsewhere. (Miguel)
>
>   - Rename config option from `RUST_KERNEL_KUNIT_TEST` to
>     `RUST_KERNEL_DOCTESTS` (and update its title), so that we can use
>     the former for the "normal" (i.e. non-doctests, e.g. `#[test]` ones)
>     tests in the future. (David)
>
>   - Follow the syntax proposed for declaring test metadata in the KTAP
>     v2 spec, which may also get used for the KUnit test attributes API.
>
>     Thus, instead of "# Doctest from line {line}", use
>     "# {test_name}.location: {file}.{line}", which ideally will allow to
>     migrate to a KUnit attribute later.
>
>     This is done in all cases, i.e. when the tests succeeds, because
>     it may be useful for users running KUnit manually, or when passing
>     `--raw_output` to `kunit.py`. (David)
>
>     David: I used "location" instead of your suggested "line" alone, in
>     order to have both in a single line, which looked nice and closer to
>     the "ASSERTION FAILURE" case/line, since now we do have the original
>     file (please see below).

I like "location" better, personally. The attributes work is still
ongoing, and while there's some benefit to having "file" and "line"
separate (it could potentially simplify some implementation on the C
side), we'll cross that bridge when we come to it.

>
>   - Figure out the original line. This is done by deploying an anchor
>     so that the difference in lines between the beginning of the test
>     and the assert, in the generated file, can be computed. Then, we
>     offset the line number of the original test, which is given by
>     `rustdoc`. (developed by Boqun)
>
>   - Figure out the original file. This is done by walking the
>     filesystem, checking directory prefixes to reduce the amount of
>     combinations to check, and it is only done once per file (rather
>     than per test).
>
>     Ambiguities are detected and reported. It does limit the filenames
>     (module names) we can use, but it is unlikely we will hit it soon
>     and this should be temporary anyway until `rustdoc` provides us
>     with the real path. (Miguel)
>
>     Tested with both in-tree and `O=` builds, but I would appreciate
>     extra testing on this one, including via the `kunit.py` script.
>

This seems to be working well on the existing cases under kunit.py
here. I'll continue to play with it, but no worries on my end thus
far.

>   - The three last items combined means that we now see this output even
>     for successful cases:
>
>         # rust_doctest_kernel_sync_locked_by_rs_0.location: rust/kernel/sync/locked_by.rs:28
>         ok 53 rust_doctest_kernel_sync_locked_by_rs_0
>
>     Which basically gives the user all the information they need to go
>     back to the source code of the doctest, while keeping them fairly
>     stable for bisection, and while avoiding to require users to write
>     test names manually. (David + Boqun + Miguel)
>
>     David: from what I saw in v2 of the RFC for the test attributes API,
>     the syntax still contains the test name when it is not a suite, so
>     I followed that, but if you prefer to omit it, please feel free to
>     do so (for me either way it is fine, and if this is the expected
>     attribute syntax, I guess it is worth to follow it to make migration
>     easier later on):
>
>         # location: rust/kernel/sync/locked_by.rs:28
>         ok 53 rust_doctest_kernel_sync_locked_by_rs_0

Thanks: while we're still arguing a bit about exactly what the format
of these will look like in the KUnit/KTAP attributes spec/patches,
what you've used matches what we've been proposing so far.

Let's stick with <test name>.location for now, and change it if needed
when the attributes spec is finalised.

>
>   - Collected `Reviewed-by`s and `Tested-by`s, except for the main
>     commit due to the substantial changes.
>
> Miguel Ojeda (7):
>   kunit: test-bug.h: include `stddef.h` for `NULL`
>   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

These are all (still) looking pretty good to me. If there are no
objections, I'd like to take these into kselftest/kunit as-is and if
we need to change anything (e.g. for consistency with attributes when
they land), do that as a follow-up.

Thanks again, Miguel, for all the work getting this going!

Cheers,
-- David

>
>  MAINTAINERS                       |   2 +
>  include/kunit/test-bug.h          |   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               |  26 +--
>  rust/kernel/kunit.rs              | 163 +++++++++++++++++++
>  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   |  72 +++++++++
>  scripts/rustdoc_test_gen.rs       | 260 ++++++++++++++++++++++++++++++
>  19 files changed, 591 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: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> --
> 2.41.0
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230718052752.1045248-1-ojeda%40kernel.org.
  

On Tue, Jul 18, 2023 at 10:38 AM David Gow <davidgow@google.com> wrote:
>
> I like "location" better, personally. The attributes work is still
> ongoing, and while there's some benefit to having "file" and "line"
> separate (it could potentially simplify some implementation on the C
> side), we'll cross that bridge when we come to it.

Yeah, I felt it looked a bit better, but if later on it ends up making
things too hard, then yeah, we can definitely simplify it.

> This seems to be working well on the existing cases under kunit.py
> here. I'll continue to play with it, but no worries on my end thus
> far.

Thanks for trying it out!

> Thanks: while we're still arguing a bit about exactly what the format
> of these will look like in the KUnit/KTAP attributes spec/patches,
> what you've used matches what we've been proposing so far.
>
> Let's stick with <test name>.location for now, and change it if needed
> when the attributes spec is finalised.

Sounds good.

> These are all (still) looking pretty good to me. If there are no
> objections, I'd like to take these into kselftest/kunit as-is and if
> we need to change anything (e.g. for consistency with attributes when
> they land), do that as a follow-up.
>
> Thanks again, Miguel, for all the work getting this going!

My pleasure -- and thanks for reviewing it so quickly and all your feedback!

Cheers,
Miguel
  

On Tue, Jul 18, 2023 at 07:27:45AM +0200, Miguel Ojeda wrote:
[...]
> 
>   - Collected `Reviewed-by`s and `Tested-by`s, except for the main
>     commit due to the substantial changes.

I've applied the series and run the following command:

	./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 --kconfig_add CONFIG_RUST=y	

everything works as expected, and I also tried modifying one of the
`assert!` to trigger it, all looks good to me. Feel free to add:

Tested-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> 
> Miguel Ojeda (7):
>   kunit: test-bug.h: include `stddef.h` for `NULL`
>   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 +
>  include/kunit/test-bug.h          |   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               |  26 +--
>  rust/kernel/kunit.rs              | 163 +++++++++++++++++++
>  rust/kernel/lib.rs                |   2 +
>  rust/kernel/str.rs                |   4 +-
>  rust/kernel/sync/arc.rs           |   9 +-
>  rust/kernel/sync/lock/mutex.rs    |   1 +
> v1: https://lore.kernel.org/rust-for-linux/20230614180837.630180-1-ojeda@kernel.org/
> v2:
> 
>   - Rebased on top of v6.5-rc1, which requires a change from
>     `kunit_do_failed_assertion` to `__kunit_do_failed_assertion` (since
>     the former became a macro) and the addition of a call to
>     `__kunit_abort` (since previously the call was done by the old
>     function which we cannot use anymore since it is a macro). (David)
> 
>   - Added prerequisite patch to KUnit header to include `stddef.h` to
>     support the `KUNIT=y` case. (Reported by Boqun)
> 
>   - Added comment on the purpose of `trait FromErrno`. (Martin asked
>     about it)
> 
>   - Simplify code to use `std::fs::write` instead of `write!`, which
>     improves code size too. (Suggested by Alice)
> 
>   - Fix copy-paste type in docs from "error" to "info" and, to make it
>     proper English, copy the `printk` docs style, i.e. from "info"
>     to "info-level message" -- and same for the "error" one. (Miguel)
> 
>   - Swap `FILE` and `LINE` `static`s to keep the same order as done
>     elsewhere. (Miguel)
> 
>   - Rename config option from `RUST_KERNEL_KUNIT_TEST` to
>     `RUST_KERNEL_DOCTESTS` (and update its title), so that we can use
>     the former for the "normal" (i.e. non-doctests, e.g. `#[test]` ones)
>     tests in the future. (David)
> 
>   - Follow the syntax proposed for declaring test metadata in the KTAP
>     v2 spec, which may also get used for the KUnit test attributes API.
> 
>     Thus, instead of "# Doctest from line {line}", use
>     "# {test_name}.location: {file}.{line}", which ideally will allow to
>     migrate to a KUnit attribute later.
> 
>     This is done in all cases, i.e. when the tests succeeds, because
>     it may be useful for users running KUnit manually, or when passing
>     `--raw_output` to `kunit.py`. (David)
> 
>     David: I used "location" instead of your suggested "line" alone, in
>     order to have both in a single line, which looked nice and closer to
>     the "ASSERTION FAILURE" case/line, since now we do have the original
>     file (please see below).
> 
>   - Figure out the original line. This is done by deploying an anchor
>     so that the difference in lines between the beginning of the test
>     and the assert, in the generated file, can be computed. Then, we
>     offset the line number of the original test, which is given by
>     `rustdoc`. (developed by Boqun)
> 
>   - Figure out the original file. This is done by walking the
>     filesystem, checking directory prefixes to reduce the amount of
>     combinations to check, and it is only done once per file (rather
>     than per test).
> 
>     Ambiguities are detected and reported. It does limit the filenames
>     (module names) we can use, but it is unlikely we will hit it soon
>     and this should be temporary anyway until `rustdoc` provides us
>     with the real path. (Miguel)
> 
>     Tested with both in-tree and `O=` builds, but I would appreciate
>     extra testing on this one, including via the `kunit.py` script.
> 
>   - The three last items combined means that we now see this output even
>     for successful cases:
> 
>         # rust_doctest_kernel_sync_locked_by_rs_0.location: rust/kernel/sync/locked_by.rs:28
>         ok 53 rust_doctest_kernel_sync_locked_by_rs_0
> 
>     Which basically gives the user all the information they need to go
>     back to the source code of the doctest, while keeping them fairly
>     stable for bisection, and while avoiding to require users to write
>     test names manually. (David + Boqun + Miguel)
> 
>     David: from what I saw in v2 of the RFC for the test attributes API,
>     the syntax still contains the test name when it is not a suite, so
>     I followed that, but if you prefer to omit it, please feel free to
>     do so (for me either way it is fine, and if this is the expected
>     attribute syntax, I guess it is worth to follow it to make migration
>     easier later on):
> 
>         # location: rust/kernel/sync/locked_by.rs:28
>         ok 53 rust_doctest_kernel_sync_locked_by_rs_0
>  rust/kernel/sync/lock/spinlock.rs |   1 +
>  rust/kernel/types.rs              |   6 +-
>  scripts/.gitignore                |   2 +
>  scripts/Makefile                  |   4 +
>  scripts/rustdoc_test_builder.rs   |  72 +++++++++
>  scripts/rustdoc_test_gen.rs       | 260 ++++++++++++++++++++++++++++++
>  19 files changed, 591 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: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> -- 
> 2.41.0
>
  

On Tue, Jul 18, 2023 at 8:00 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> I've applied the series and run the following command:
>
>         ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 --kconfig_add CONFIG_RUST=y
>
> everything works as expected, and I also tried modifying one of the
> `assert!` to trigger it, all looks good to me. Feel free to add:
>
> Tested-by: Boqun Feng <boqun.feng@gmail.com>

Thanks a lot for trying it, Boqun!

David/Shuah: I noticed this "Tested-by" tag is not in the applied
commits. If you happen to rebase, it would be nice to pick it up.
Thanks!

Cheers,
Miguel