Date:   Tue, 25 Apr 2023 19:12:29 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Maxime Ripard <maxime@...no.tech>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Stephen Boyd <sboyd@...nel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Rae Moar <rmoar@...gle.com>,
        Benjamin Berg <benjamin@...solutions.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Jonathan Cameron <jic23@...nel.org>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com
Subject: Re: [PATCH v1 1/3] kunit: Add kunit_add_action() to defer a call
 until test exit

On Fri, Apr 21, 2023 at 1:42 AM David Gow <davidgow@...gle.com> wrote:
>
> Many uses of the KUnit resource system are intended to simply defer
> calling a function until the test exits (be it due to success or
> failure). The existing kunit_alloc_resource() function is often used for
> this, but was awkward to use (requiring passing NULL init functions, etc),
> and returned a resource without incrementing its reference count, which
> -- while okay for this use-case -- could cause problems in others.
>
> Instead, introduce a simple kunit_add_action() API: a simple function
> (returning nothing, accepting a single void* argument) can be scheduled
> to be called when the test exits. Deferred actions are called in the
> opposite order to that which they were registered.
>
> This mimics the devres API, devm_add_action(), and also provides
> kunit_remove_action(), to cancel a deferred action, and
> kunit_release_action() to trigger one early.

Apologies for the delayed bikeshedding.

I think mimicking the devres API is a better idea than kunit_defer()
and friends.
But I can't help but think this still isn't the best name.
I personally would have no idea what `kunit_release_action()` does
without looking it up.

I feel like `kunit_add_cleanup()` probably works better for a unit
test framework.
I think `kunit_remove_cleanup()` is fine and `kunit_release_cleanup()`
is questionably ok.
Instead of `release`, maybe it should be `kunit_trigger_cleanup()` or
more verbosely, something like `kunit_early_trigger_cleanup()`.

I tried to look for equivalents in other languages/frameworks:
* Rust and C++ rely on RAII, don't think they have equivalents in testing libs
* Python has `self.addCleanup()`,
https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup
* Go has `t.Cleanup()`, https://pkg.go.dev/testing#T.Cleanup
* Looking at Zig since it also has a `defer`, I guess they just use
that, I don't see anything in
https://ziglang.org/documentation/master/std/#A;std:testing
* I know nothing about JUnit, but a quick search seems like they rely
on @After and @AfterClass annotations,
https://junit.org/junit4/javadoc/4.12/org/junit/After.html
* I know even less about HUnit, but it looks like it relies on
wrapping things via the IO monad,
https://hackage.haskell.org/package/HUnit-1.6.2.0/docs/Test-HUnit-Base.html#t:AssertionPredicate
* Since we were inspired by TAP, I tried to look at Perl, but didn't
immediately see anything that looked equivalent,
https://metacpan.org/pod/Test::Most

>
> This is implemented as a resource under the hood, so the ordering
> between resource cleanup and deferred functions is maintained.
>
> Signed-off-by: David Gow <davidgow@...gle.com>
> ---

<snip>

> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index c0d88b318e90..6db28cd43e9b 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -387,4 +387,80 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
>   */
>  void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
>
> +
> +/**
> + * kunit_add_action() - Defer an 'action' (function call) until the test ends.
> + * @test: Test case to associate the action with.
> + * @func: The function to run on test exit
> + * @ctx: Data passed into @func
> + *
> + * Defer the execution of a function until the test exits, either normally or
> + * due to a failure.  @ctx is passed as additional context. All functions
> + * registered with kunit_add_action() will execute in the opposite order to that
> + * they were registered in.
> + *
> + * This is useful for cleaning up allocated memory and resources.

Re renaming to kunit_add_cleanup(), I think this makes writing the
comment easier.

E.g.
- kunit_add_action() - Defer an 'action' (function call) until the test ends.
+ kunit_add_cleanup() - Call a function when the test ends.
+ ...
+ This is useful for cleaning up allocated memory and resources.

Daniel

Powered by blists - more mailing lists