Re: [PATCH 2/7] rust: kunit: support checked `-> Result`s in KUnit `#[test]`s

David Gow
2025-05-06T06:32:58Z

[-- Attachment #1: Type: text/plain, Size: 3300 bytes --]

On Tue, 6 May 2025 at 03:34, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, May 05, 2025 at 02:02:09PM +0800, David Gow wrote:
> > On Sat, 3 May 2025 at 05:51, Miguel Ojeda <ojeda@kernel.org> wrote:
> > >
> > > Currently, return values of KUnit `#[test]` functions are ignored.
> > >
> > > Thus introduce support for `-> Result` functions by checking their
> > > returned values.
> > >
> > > At the same time, require that test functions return `()` or `Result<T,
> > > E>`, which should avoid mistakes, especially with non-`#[must_use]`
> > > types. Other types can be supported in the future if needed.
> > >
> > > With this, a failing test like:
> > >
> > > #[test]
> > > fn my_test() -> Result {
> > > f()?;
> > > Ok(())
> > > }
> > >
> > > will output:
> > >
> > > [ 3.744214] KTAP version 1
> > > [ 3.744287] # Subtest: my_test_suite
> > > [ 3.744378] # speed: normal
> > > [ 3.744399] 1..1
> > > [ 3.745817] # my_test: ASSERTION FAILED at rust/kernel/lib.rs:321
> > > [ 3.745817] Expected is_test_result_ok(my_test()) to be true, but is false
> > > [ 3.747152] # my_test.speed: normal
> > > [ 3.747199] not ok 1 my_test
> > > [ 3.747345] not ok 4 my_test_suite
> > >
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > ---
> >
> > This is a neat idea. I think a lot of tests will still want to go with
> > the () return value, but having both as an option seems like a good
> > idea to me.
> >
>
> Handling the return value of a test is definitely a good thing, but I'm
> not sure returning `Err` should be treated as assertion failure
> though. Considering:
>
> #[test]
> fn foo() -> Result {
> let b = KBox::new(...)?; // need to allocate memory for the test
> use_box(b);
> assert!(...);
> }
>
> if the test runs without enough memory, then KBox::new() would return a
> Err(ENOMEM), and technically, it's not a test failure rather the test
> has been skipped because of no enough resource.
>
> I would suggest we handle the `Err` returns specially (mark as "SKIPPED"
> maybe?).
>
> Thoughts?
>
> Regards,
> Boqun
>

FWIW, having out-of-memory situations trigger a test failure is
consistent with what other KUnit tests (written in C) do.

There's both advantages and disadvantages to this: on the one hand,
it's prone to false positives (as you mention), on the other, it
catches cases where the test is using an unusually large amount of
memory (which could indeed be a test issues).

My go-to rule is that tests should fail if small allocations (which,
in the normal course of events, _should_ succeed) fail, but if they
have unusual resource requirements (beyond "enough memory for the
system to run normally") these should be checked separately when the
test starts.

That being said, I definitely think that, by default, an `Err` return
should map to a FAILED test result, as not all Err Results are a
resource exhaustion issue, and we definitely don't want to mark a test
as skipped if there's a real error occurring. If test authors wish to
skip a test when an out-of-memory condition occurs, they probably
should handle that explicitly. (But I'd not be opposed to helpers to
make it easier.)

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]