Allow to run unit tests using KUnit with a user-space like syntax by JoseExposito · Pull Request #950 · Rust-for-Linux/linux (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation53 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Why?

I'm working on the Rust abstractions for the HID subsystem and on a HID driver.

In order to be able to test the HID subsystem abstractions, I need to avoid calling the C APIs, so I created a bindings mock module that I import as:

#[cfg(CONFIG_KUNIT)] use crate::hid::bindings_mock as bindings;

Which returns well known values for my tests. For reference, this is how the driver adapter test looks like:

#[kunit_tests(rust_kernel_hid_driver)] mod tests { use super::; use crate::{c_str, driver, hid, prelude::}; use core::ptr;

struct SimpleTestDriver;
impl Driver for SimpleTestDriver {
    type Data = ();
}

#[test]
fn rust_test_hid_driver_adapter() {
    let mut hid = bindings::hid_driver::default();
    let name = c_str!("SimpleTestDriver");
    static MODULE: ThisModule = unsafe { ThisModule::from_ptr(ptr::null_mut()) };

    let res = unsafe {
        <hid::Adapter<SimpleTestDriver> as driver::DriverOps>::register(&mut hid, name, &MODULE)
    };
    assert_eq!(res, Err(ENODEV)); // The mock returns -19
}

}

While I could do the same using documentation tests, using regular tests allows to reduce code duplication.

About the patches

The first patch introduces the low level macros kunit_case! and kunit_test_suite!. It is based on the work mentioned by @sulix in #935 (comment). David, I added you as Co-developed-by, let me know if that works for you, please.

The second patch introduces a procedural macro that allows to run the tests almost with the same syntax used in user-space. The only change required is replacing #[cfg(test)] with #[kunit_tests(kunit_test_suit_name)].

Possible issues/questions

The series relies on #[cfg(CONFIG_KUNIT)] being defined only while running the tests. A similar approach is used by static stub patches, so I hope that it also works here, but it'd be nice to hear David's opinion.
Thanks to David for clarifying this point, it is solved now.
As far as I know, procedural macros don't support $crate and I was not able to find a workaround to be able to use this macro both by the kernel crate and in drivers. In the macro, I use code like crate::bindings::<...> that won't work when used by a driver. Do you know if it is possible to fix this?

Thanks very much for doing this — it worked fine for me here.

A few comments:

The first patch introduces the low level macros kunit_case! and kunit_test_suite!. It is based on the work mentioned by @sulix in #935 (comment). David, I added you as Co-developed-by, let me know if that works for you, please.

Works fine for me: thanks for tidying it all up!

The second patch introduces a procedural macro that allows to run the tests almost with the same syntax used in user-space. The only change required is replacing #[cfg(test)] with #[kunit_tests(kunit_test_suit_name)].

While I'm not familiar enough to comment on proc macro implementation, I quite like the syntax here personally.

The series relies on #[cfg(CONFIG_KUNIT)] being defined only while running the tests. A similar approach is used by static stub patches, so I hope that it also works here, but it'd be nice to hear David's opinion.

I'm afraid that's not something you can rely on in all circumstances. There are some setups which unconditionally compile KUnit as a module (I think Red Hat and Android are going down this path), as well as the possibility of wanting to run the tests and the real driver on the same system.

The "proper" way to check if a test is running is by using the combination of CONFIG_KUNIT and checking current->kunit_test is not NULL. This allows, e.g., stubs to only be active on KUnit's threads.

The current version of the stubs implementation (which you linked) does not work if CONFIG_KUNIT=m, but that should be fixed in the next revision.

tl;dr: If CONFIG_KUNIT is not set, there are definitely no tests, but it's not true to say that CONFIG_KUNIT is only set while tests are running: it's perfectly possible (even if it's, IMHO, a terrible idea) to set CONFIG_KUNIT=y and then use a kernel normally.

Further thoughts (which could be relegated to follow-up patches if you prefer) include:

Could we include a sample test to make sure these work? Something simple this would do to start (and more tests could be added as more features are supported):

#[kunit_tests(kunit_rust)]
mod tests {
    #[test]
    fn example_test() {
        assert_eq!(1+1, 2);
    }
}

It'd be nice to expose the KUnit assert/expect macros as an alternative to Rust's built-in ones. The KUnit tooling doesn't handle the failed rust assertions properly, treating it as a crash. And expectations are probably nicer, as exiting a test early is not handled well with KUnit/Rust (see How to handle Kunit killing the current thread on assertion failures? #759).
Unlike with the doctests' implementation, I don't see a way to access the struct kunit object. I suspect that using current->kunit_test is the right way to handle that.
It may make sense to start to use some of these macros for (and otherwise share code with) the doctests implementation, too.

Regardless, this is looking pretty good to me, thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may make more sense for us to just outline the suite name generally, so that c_str!() and similar can be used (and we could get rid of the length limit as well). That'd require changes to the C code as well, but I don't think that'd be a problem.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, changing the name field from char[256] to char * would simplify this code a little bit. In fact (probably because I'm a Rust noob) casting this field was the most time consuming part of this PR 😅

For the sake of simplicity, I don't mind keeping this code. Changing C code (upstream and here) would require backporting and might slow down the inclusion of this PR. But if you want to change the field type, let me know and I'll submit a patch upstream.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you: I'm happy to accept a change upstream to change this if you'd prefer. Otherwise, it's something we can either leave as-is or change later, once everything's accepted upstream.

I'd (optimistically) assumed that very little rust stuff would be backported to earlier kernel versions, so as long as we can stay relatively in-sync between the Rust tree and the KUnit one, it shouldn't be too horrific. If you're planning to either backport to much older releases, or to try to push a driver depending on this via another branch in the short term, though, that could be trickier…

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused -- originally I thought José meant back merges, i.e. to bring the C change into the rust branch here. That is something I did before the merge every few weeks, and we were discussing the other day whether to continue doing it until the tree is ready to be decommissioned.

But then David mentions backporting to older release versions: indeed, that is not something we plan to do. (Not that I wouldn't be glad to be in a situation where we could think about it -- it would mean Rust was wildly successful in the kernel :)

Which one of the two are we discussing?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused -- originally I thought José meant back merges, i.e. to bring the C change into the rust branch here.

This is what I meant. While applying the patch both upstream and here is trivially easy, I'd prefer to keep this PR as simple as possible and avoid unnecessary interdependence.

I'm working a more complex series based on this PR and, hopefully, keeping things as simple as possible, will help getting it merged.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah: I was a bit unsure which was meant, so tried to address both.

How about we take this PR in as-is (with the char[256] array), and when it lands in Linus' tree, we can decide if changing it is worthwhile? That'll avoid any complications.

Hi David,

Thanks a lot for your comments! You really helped me understand some points.

tl;dr: If CONFIG_KUNIT is not set, there are definitely no tests, but it's not true to say that CONFIG_KUNIT is only set while tests are running: it's perfectly possible (even if it's, IMHO, a terrible idea) to set CONFIG_KUNIT=y and then use a kernel normally.

While this doesn't affect this merge request, it complicates a bit creating mocks.

It means that we won't be able to remove the mock code at compile time, introducing a bit of run-time overhead checking for current->kunit_test :(

Next weekend, I'll try to add an example of how mocking a module could work in Rust. Hopefully it'll make easier to understand the problematic.

Could we include a sample test to make sure these work?

Sure, good point.

It'd be nice to expose the KUnit assert/expect macros as an alternative to Rust's built-in ones. The KUnit tooling doesn't handle the failed rust assertions properly, treating it as a crash. And expectations are probably nicer, as exiting a test early is not handled well with KUnit/Rust (see How to handle Kunit killing the current thread on assertion failures? #759).

I could try to have a look into that in the future,. Hopefully, it shouldn't be too complicated.
But I'd like to finish the driver that originated this MR first. Given that I do kernel stuff mostly on the weekends, it could take me a while...

Unlike with the doctests' implementation, I don't see a way to access the struct kunit object. I suspect that using current->kunit_test is the right way to handle that.

I removed the test parameter from the signature on purpose to keep test cases as similar as possible to user-space.

I think that using current->kunit_test is the best option as well. Ideally, the KUnit assert/expect Rust macros should get current->kunit_test hiding the extra complexity.

Thanks again for reviewing this! I'll address your comments as soons as possible as submit v2.

Hi @sulix,

I just pushed v2 of the patches.

In addition to addressing your comments, this second version introduces a new patch adding the is_kunit_test() function.

This function checks CONFIG_KUNIT and current->kunit_test to decide if we are in a KUnit test execution or not. I noticed that in your stub series you are using kunit_get_current_test(), which is faster.

Unfortunately, kunit_get_current_test() is not available yet in the Rust branch, so for the moment I'll kept it as it is and once the changes from upstream are backported I'll update my function.

Also, I included a couple of examples/Kunit tests to show how to mock a function and an entire module. The module mocked in the example is bindings, a module most abstractions developers would be interested in mocking.

Thanks again for looking into this,
Jose

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I'm pretty happy with this. The only real change I'd suggest is renaming is_kunit_test() to in_kunit_test(), to make it clear that we're running in a test context. (But if there's a strong reason to keep is_kunit_test(), I could live with it…)

Otherwise, this looks pretty good. I'm not going to pretend to be enough of a rust expert to have caught issues with all of the code properly (particularly the proc macros), but from a KUnit point of view, I'm happy with it.

(One, slightly related note for the future, is that we're going to add a more generic way of having "hooks" where non-test code calls into KUnit, à la kunit_get_current_test()/in_kunit_test(). This is pretty macro heavy at the moment, so if it's going to make doing future things in rust really difficult (due to, e.g., bindgen), feel free to complain / suggest changes. It's more a problem for a future patch, though (and I suspect it'd need wrapping / redoing to be more idiomatic anyway), so it's not really a problem for now.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you: I'm happy to accept a change upstream to change this if you'd prefer. Otherwise, it's something we can either leave as-is or change later, once everything's accepted upstream.

};
}

/// In some cases, you need to call test-only code from outside the test case, for example, to

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to distinguish between this sort of "mocking", which only replaces direct calls with other KUnit mocking functionality, like the upcoming static stub feature:
https://lore.kernel.org/linux-kselftest/20221208061841.2186447-1-davidgow@google.com/

The main differences are that the static stub feature redirects all calls to a function, even if it's indirect (e.g., from a different module, crate, file, etc). It also automatically does the in_kunit_test() check, and the replacement can be swapped in and out at runtime.

To be clear, I think this is fine as-is, but we will want to come up with a consistent story about when to use which features (and probably a more consistent naming scheme) once the static stub patches land.

In the meantime, we've used the term "function redirection" to refer to the general concept of replacing a function, and then had more specific names for different implementations (mostly "stubbing"-related). A somewhat-outdated document gives some of the rationale behind this, mostly that "mocking" as a term is a bit overloaded.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an entry in my to-do list to document mocking and add a link to the general KUnit docs in #935 once this PR gets merged.

At work, I write JavaScript and the terminology makes sense to me. It's similar to the one used by the popular testing frameworks.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah: personally, I'd rather we focus this PR on getting the basics of running tests going, and leave any extensive documentation of mocking or related things to a separate one.

Terminology-wise, I'm not too worried about matching that old document, which was mostly based on an abandoned "mocking" proposal which had some more advanced "mocking" features, like autogenerating stubs, and being able to assert easily that a given call will receive certain arguments. Since that was abandoned, and the "stubs" system (which is much simpler) hasn't quite landed yet, none of the nomenclature here is set in stone.

I don't think we need to hold this PR back over some slightly inconsistent, but not actually conflicting, terminology in a comment. If "mock" works well here, there's no reason we can't adopt it for the C side either.

#[test]
fn rust_test_kunit_kunit_tests() {
let running = true;
assert_eq!(running, true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I think that someday we'll want to be able to use the KUNIT assertion system (and perhaps encourage more use of expectations over assertions, due to the issues with stack unwinding), but I think this can definitely be a separate patch.

This is pretty macro heavy at the moment, so if it's going to make doing future things in rust really difficult (due to, e.g., bindgen), feel free to complain / suggest changes.

I don't think we need to constraint the C side here. So unless it is a blocker, or something that is anyway an improvement for both sides (like the const correctness one I sent once), I would say we should try to find the best way for the C side, and then see if we can work with that.

I don't think we need to constraint the C side here. So unless it is a blocker, or something that is anyway an improvement for both sides (like the const correctness one I sent once), I would say we should try to find the best way for the C side, and then see if we can work with that.

Agreed: we're still very much experimenting with the C version here. (Whatever we get will just be a collection of function pointers in some sense), so I doubt it'd cause serious issues.

That being said, I'd really like it if, in general, we can keep the C and Rust versions of the KUnit API as similar as possible. And since Rust has a lot of constraints to make APIs safe, we're more than willing to adjust the C API to make it more "Rust friendly" where it makes sense to do so.

This patchset is (with the possible exception of patch 1, which I probably shouldn't review as the co-author :-P)

Reviewed-by: David Gow <davidgow@google.com>

Assuming the Rust experts don't find anything wrong with it, I'm happy for it to go in as-is.

Thanks!
— David

That being said, I'd really like it if, in general, we can keep the C and Rust versions of the KUnit API as similar as possible.

Definitely! I was just trying to give you as much freedom as possible :)

Hi again, and thanks a lot for all of your comments.

I pushed v3 addressing them:

Add Reviewed-by: David Gow davidgow@google.com
Rename kunit_test_suite to kunit_unsafe_test_suite and document the safety requirements
Wrap kunit_unsafe_test_suite body's in const _: () = { ... };
Make the kernel crate available to kunit_tests

v3 still works for me, thanks!

I'd've had a slight preference for maintaining the old kunit_test_suite name, instead of renaming it to kunit_unsafe_test_suite, so that it matches the C API, and just marking the macro unsafe, but it looks like that's not possible. So I'm okay with it as-is.

Thanks again for working on this!

Thanks a lot David!

After rebasing a branch on this one I noticed that the build fails when CONFIG_KUNIT was not set, so I removed the warning in rust/kernel/lib.rs:

+#[allow(unused_extern_crates)] extern crate self as kernel;

So I fixed it in v4. Sorry for the noise.

macro_rules! kunit_unsafe_test_suite {
($name:ident, $test_cases:ident) => {
const _: () = {
static KUNIT_TEST_SUITE_NAME: [i8; 256] = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be const, but keeping it as static is fine foo.

v4 still works for me:

./tools/testing/kunit/kunit.py run --arch x86_64 --kconfig_add CONFIG_RUST=y --make_options LLVM=1 rust_kernel_kunit passes both tests.
Both tests also pass on UML with the appropriate patches backported from the UML tree.
make defconfig + CONFIG_RUST=y (- the DRM subsystem, so it'd build quicker) still builds.
make tinyconfig (no Rust support) still builds.

Thanks again for continuing to push this!

Add a couple of Rust macros to allow to develop KUnit tests without relying on generated C code:

The kunit_unsafe_test_suite! Rust macro is similar to the kunit_test_suite C macro.
The kunit_case! Rust macro is similar to the KUNIT_CASE C macro. It can be used with parameters to generate a test case or without parameters to be used as delimiter in kunit_test_suite!.

While these macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.

Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com Signed-off-by: José Expósito jose.exposito89@gmail.com

Add a new procedural macro (#[kunit_tests(kunit_test_suit_name)]) to run KUnit tests using a user-space like syntax.

The macro, that should be used on modules, transforms every #[test] in a kunit_case! and adds a kunit_unsafe_test_suite! registering all of them.

The only difference with user-space tests is that instead of using #[cfg(test)], #[kunit_tests(kunit_test_suit_name)] is used.

Note that #[cfg(CONFIG_KUNIT)] is added so the test module is not compiled when CONFIG_KUNIT is set to n.

Reviewed-by: David Gow davidgow@google.com Signed-off-by: José Expósito jose.exposito89@gmail.com

In some cases, you need to call test-only code from outside the test case, for example, to mock a function or a module.

In order to check whether we are in a test or not, we need to test if CONFIG_KUNIT is set. Unfortunately, we cannot rely only on this condition because some distros compile KUnit in production kernels, so checking at runtime that current->kunit_test != NULL is required.

Note that the C function kunit_get_current_test() can not be used because it is not present in the current Rust tree yet. Once it is available we might want to change our Rust wrapper to use it.

This patch adds a function to know whether we are in a KUnit test or not and examples showing how to mock a function and a module.

Reviewed-by: David Gow davidgow@google.com Signed-off-by: José Expósito jose.exposito89@gmail.com

Hi everyone,

Sorry for the long delay addressing the 2 remaining issues. I've being swamped with work since the year started.

I pushed v5 adding a missing unsafe comment and declaring KUNIT_TEST_SUITE as UnsafeCell to avoid UB.

Thanks a lot to everyone for your patience and feedback.

Thanks for rebasing and sending upstream the patches David :D

matthewtgilbride added a commit to matthewtgilbride/linux that referenced this pull request

May 17, 2024

…e like syntax"

David Gow davidgow@google.com says:

This series was originally written by José Expósito, and can be found here: Rust-for-Linux#950

Add support for writing KUnit tests in Rust. While Rust doctests are already converted to KUnit tests and run, they're really better suited for examples, rather than as first-class unit tests.

This series implements a series of direct Rust bindings for KUnit tests, as well as a new macro which allows KUnit tests to be written using a close variant of normal Rust unit test syntax. The only change required is replacing '#[cfg(test)]' with '#[kunit_tests(kunit_test_suite_name)]'

An example test would look like: #[kunit_tests(rust_kernel_hid_driver)] mod tests { use super::; use crate::{c_str, driver, hid, prelude::}; use core::ptr;

    struct SimpleTestDriver;
    impl Driver for SimpleTestDriver {
        type Data = ();
    }

    #[test]
    fn rust_test_hid_driver_adapter() {
        let mut hid = bindings::hid_driver::default();
        let name = c_str!("SimpleTestDriver");
        static MODULE: ThisModule = unsafe { ThisModule::from_ptr(ptr::null_mut()) };

        let res = unsafe {
            <hid::Adapter<SimpleTestDriver> as driver::DriverOps>::register(&mut hid, name, &MODULE)
        };
        assert_eq!(res, Err(ENODEV)); // The mock returns -19
    }
}

Changes since the GitHub PR:

Rebased on top of kselftest/kunit
Add const_mut_refs feature This may conflict with https://lore.kernel.org/lkml/20230503090708.2524310-6-nmi@metaspace.dk/
Add rust/macros/kunit.rs to the KUnit MAINTAINERS entry

Link: https://lore.kernel.org/r/20230720-rustbind-v1-0-c80db349e3b5@google.com

ojeda mentioned this pull request

Aug 27, 2024

16 tasks

tgross35 added a commit to tgross35/rust that referenced this pull request

Sep 5, 2024

…tmcm

clarify that addr_of creates read-only pointers

Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with rust-lang#56604 and other UCG discussions. Also see [this collection of links](Rust-for-Linux/linux#950 (comment)) where rustc treats addr_of! as a "non-mutating use".

So, let's better be careful for now.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 5, 2024

…tmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 5, 2024

…tmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 5, 2024

…tmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 5, 2024

…tmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Sep 6, 2024

…tmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Sep 6, 2024

…tmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Sep 6, 2024

Rollup merge of rust-lang#129653 - RalfJung:addr-of-read-only, r=scottmcm

clarify that addr_of creates read-only pointers

So, let's better be careful for now.

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request

Sep 25, 2024

clarify that addr_of creates read-only pointers

Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with rust-lang/rust#56604 and other UCG discussions. Also see [this collection of links](Rust-for-Linux/linux#950 (comment)) where rustc treats addr_of! as a "non-mutating use".

So, let's better be careful for now.

Closing this MR now that, thanks to the big effort from @sulix, this is available upstream.

Thanks a lot for the huge effort upstreaming this David!