-
Notifications
You must be signed in to change notification settings - Fork 430
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to run unit tests using KUnit with a user-space like syntax #950
base: rust
Are you sure you want to change the base?
Allow to run unit tests using KUnit with a user-space like syntax #950
Conversation
Thanks very much for doing this — it worked fine for me here. A few comments:
Works fine for me: thanks for tidying it all up!
While I'm not familiar enough to comment on proc macro implementation, I quite like the syntax here personally.
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:
Regardless, this is looking pretty good to me, thanks! |
rust/kernel/kunit.rs
Outdated
} | ||
|
||
ret | ||
}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 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.
Sure, good point.
I could try to have a look into that in the future,. Hopefully, it shouldn't be too complicated.
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 Thanks again for reviewing this! I'll address your comments as soons as possible as submit v2. |
49b40c8
to
e8fd415
Compare
Hi @sulix, I just pushed v2 of the patches. In addition to addressing your comments, this second version introduces a new patch adding the This function checks Unfortunately, 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 Thanks again for looking into this, |
There was a problem hiding this comment.
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.)
rust/kernel/kunit.rs
Outdated
} | ||
|
||
ret | ||
}; |
There was a problem hiding this comment.
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…
}; | ||
} | ||
|
||
/// In some cases, you need to call test-only code from outside the test case, for example, to |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
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 |
e8fd415
to
f1b1719
Compare
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)
Assuming the Rust experts don't find anything wrong with it, I'm happy for it to go in as-is. Thanks! |
Definitely! I was just trying to give you as much freedom as possible :) |
f1b1719
to
6251a35
Compare
Hi again, and thanks a lot for all of your comments. I pushed v3 addressing them:
|
v3 still works for me, thanks! I'd've had a slight preference for maintaining the old Thanks again for working on this! |
6251a35
to
1f637ef
Compare
Thanks a lot David! After rebasing a branch on this one I noticed that the build fails when +#[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] = { |
There was a problem hiding this comment.
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:
Thanks again for continuing to push this! |
1f637ef
to
84b0fc2
Compare
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 Thanks a lot to everyone for your patience and feedback. |
FYI, now that the doctests support has landed in the kselftest/kunit branch, I've gone ahead and rebased this and sent it out: With some luck, we'll be able to get it in 6.6. |
Thanks for rebasing and sending upstream the patches David :D |
…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
…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.
…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.
…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.
…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.
…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.
…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.
…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.
Rollup merge of rust-lang#129653 - RalfJung:addr-of-read-only, r=scottmcm 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.
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.
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:
Which returns well known values for my tests. For reference, this is how the driver adapter test looks like:
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!
andkunit_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 likecrate::bindings::<...>
that won't work when used by a driver. Do you know if it is possible to fix this?